Module Hierarchy Refactor #469

ducky64 · 2017-01-31T03:05:30Z

Implementation for the proposal in #427

Major changes:

Implemented the new module hierarchy in Special Top Module class to rename/delete clock/reset #427 (abstract BaseModules, BlackBoxes, UserModules without implicit io, clock, reset, and ImplicitModule which is chisel's old Module)
UserModules have ios defined with IO(...), that is, IO(...) registers its contents as a module IO. Arbitrary numbers of IO are allowed.
All IOs are named by reflection, so they must be of the form val myIo = IO(...). IO names are no longer hardcoded.
Refactoring to make it easier to reason about correctness and for better abstraction boundaries.
Added getReset, getClock, forcedReset, forcedClock to Builder. These return empty (or error out) when in a UserModule without an implicit clock / reset. This also paves the way for withClock blocks.
Yes, rocket chip still builds.

Debate points:

Should io be required in ImplicitModule? Perhaps io should be more a convention (or even have IOs turned into the wild wild west)?
Blackbox io handling is inconsistent with Module io. Blackboxes don't emit the io prefix when elaborating; this was likely to allow it to do something useful before we even considered having IOs not in io.. Now that this can change - should it? (as in, instead of putting things in an io bundle, just dump it directly into the BlackBox as IO(...)?
This will break override io (except in compatibility mode, the auto-wrap handles things properly), because it seems that every time you define val io = IO(...), even if overridden, it calls the IO and creates a port. The overridden ones are unable to get a name through reflection and fails.
Should it fail if it can't name an IO through reflection? It can have a generic name, like port_1, ... On the other hand, some use cases may require good names, like if you had a top-level Verilog file instantiating a Chisel-generated module. Additionally, we rely on reflection for Bundle elements.

sdtwigg · 2017-01-31T03:52:12Z

A lot of places take a BaseModule (like in connect signature) when they should be taking the more descriptive UserModule.

Also, mandating reflection works and having multiple IOs seems a bit too risky: would likely be best to still have 1 IO. Different Module types just may interpret that IO differently (for example, in a raw module, that IO is the true IO whereas in ImplicitModule that IO is a subpart io of the true io which includes clock and reset).

As noted, the current approach with IO() is inconsistent with BlackBoxes (and would make it tricky to have programmatic IOs using Record....)

ducky64 · 2017-01-31T06:16:38Z

Yeah, I guess the context_mod should be UserModules since it really doesn't make sense to be issuing connects in a BlackBox. Will fix.

Mandating reflection: we already do that in Bundles, so there is precedent. It also uses very similar infrastructure. Whether we should have multiple IO (or whether it's worth having another necessary reflection based system) is a larger question. There have been requests for not needing the .io fields.

Record support is a good point for Blackbox IOs. Though style-wise current Blackbox IOs are already inconsistent with Module IOs (and it shows through the code structure).

sdtwigg · 2017-01-31T07:35:19Z

My discussions with other people who use Scala and have done DSL work (but are not involved with Chisel) have reaffirmed avoiding using reflection (particularly runtime reflection) for things.

Also, as discovered, the Bundle reflection is already somewhat fragile and people are more likely to have complex Modules than complex Bundles.

sdtwigg · 2017-01-31T07:38:34Z

Addendum: This is a good opportunity to bring BlackBox IO's in line with other Module IO's. The desires are also similar: for both you want fine-grained control of IO naming.

ducky64 · 2017-01-31T07:46:13Z

We can have the old Blackbox behavior preserved in the compatibility layer.

Though what should the new behavior be? We don't want the io prefix so Chisel Blackboxes can interface with native Verilog modules without a wrapper layer, but reflection-based IO naming (along with the lack of Record-like programmatic generation) is also undesirable. If we just dump the io contents into the top level, that risks a conflict with the ImplicitModule clock and reset signals.

sdtwigg · 2017-01-31T07:50:39Z

Submodules of BaseModule decide their IO naming structure.

For BlackBox and RawModule, we have them dump their io contents as the raw port names.
For ImplicitModule, the whole io Record is just labeled io (as if it was a field of the effective true io).

ducky64 · 2017-01-31T07:51:54Z

Isn't that what we have now?

sdtwigg · 2017-01-31T07:58:13Z

I haven't worked through your code but my impression is that:

class TestModuleR extends RawModule {
  val io = IO(new Bundle {
    val clk = Input(Clock())
    val in = Input(Bool())
    val out = Output(Bool())
  })
}

in my proposal would result in 3 ports called "clk", "in", and "out"
whereas in your proposal results in 3 ports called "io_clk", "io_in", "io_out" where the 'io' is being derived reflectively. (you would instead need 3 vals called clk, in, and out all wrapped in IO)

ducky64 · 2017-01-31T08:09:43Z

Ah, I see, RawModule is a sibling class to ImplicitModule rather than users inheriting UserModule.

It could be done, though it's also inconsistent with ImplicitModule, which I expect to see the most usage because of the implicitly threaded clock and reset.

Would be nice to have some way to unify all three, though that might involve mangling the clock and reset naming for ImplicitModule in the event of a conflict. Which all things considered, might not be all that bad?

ducky64 · 2017-02-25T03:15:01Z

Updated with latest resolution.
ExtModule and MultiIOModule (actually an alias for chisel3.core.ImplicitModule) implemented.
Module is an alias for chisel3.core.LegacyModule, which is an ImplicitModule that enforces that the only IOs are io, clock, and reset. While MultiIOModule requires reflection for naming IOs, (Legacy)Module does not.

ducky64 · 2017-02-25T03:16:27Z

Also added exposed RawModule (which is just a UserModule). I'm not sure what kind of tests we want to see there, it's more absence of features than presence of any feature.

ducky64 · 2017-03-21T00:48:41Z

Bump on review...

sdtwigg

Lacks tests. Certain constructs make it clear that there was never an attempt to instantiate an ImplicitModule or LegacyModule inside a non-ImplicitModule.

sdtwigg · 2017-03-15T19:26:44Z

chiselFrontend/src/main/scala/chisel3/core/BiConnect.scala

@@ -121,12 +121,12 @@ object BiConnect {

  // This function checks if element-level connection operation allowed.
  // Then it either issues it or throws the appropriate exception.
-  def elemConnect(implicit sourceInfo: SourceInfo, connectCompileOptions: CompileOptions, left: Element, right: Element, context_mod: Module): Unit = {
+  def elemConnect(implicit sourceInfo: SourceInfo, connectCompileOptions: CompileOptions, left: Element, right: Element, context_mod: UserModule): Unit = {


You can drastically simplify this function (at least for the sink := source behavior variant) by just asserting that the sink is writeable and the source is readable then issuing. Then, you don't need to match on all these cases and subcases.

I think this code improvement is orthogonal to the module refactor, this can be done as a follow-on PR.

sdtwigg · 2017-03-15T19:39:39Z

src/main/scala/chisel3/compatibility.scala

@@ -152,16 +152,43 @@ package object Chisel {     // scalastyle:ignore package.object.name
  object Bool extends BoolFactory
  val Mux = chisel3.core.Mux

-  type BlackBox = chisel3.core.BlackBox
+  import chisel3.core.Param
+  abstract class BlackBox(params: Map[String, Param] = Map.empty[String, Param]) extends chisel3.core.BlackBox(params) {


Comments on what this is supposed to be doing distinct from BlackBox would be good.

Done (this auto wraps IO)

sdtwigg · 2017-03-21T02:47:50Z

chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala

+
+    val names = nameIds(classOf[ExtModule])
+
+    // Name ports based on reflection


It is a step backwards to now be relying on the reflection to work in order to get a functionally correct circuit.

But you get consistency in return, in that the io. prefix isn't being silently dropped.

Again, this is experimental, and we'll see whether people like this or the old style more. I don't think reflection has been a problem, and checks ensure that it will never silently drop a port.

I refuse to accept the claim that use of reflection has not been a problem. One came up with Bundles (which classically have less implementation trickery than modules since users would come to understand that reflection was being used to inspect them) literally days before you proposed expanding the use of reflection (#456).

And we will fix them as they come up, just like with any Chisel bugs. I don't know of any fundamental issues with reflections that would preclude some functionality.

sdtwigg · 2017-03-21T02:49:24Z

chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala

+  *   val O = IO(Output(Clock()))
+  *   val I = IO(Input(Clock()))
+  *   val IB = IO(Input(Clock()))
+  * }


Having to separate each IO into its own val with IO wrapping is more verbose than before....

What is the benefit?

Does this mean users will now have to dot into arbitrary class members, rather than just io?

Furthermore, why are all the I/Os clock? There's nothing clock-related with IBUFDS.

ExtModule is an experimental API and aims for consistency with regular Module (where you have the io. prefix for io members; old BlackBox dropped the io. prefix when emitting Verilog).

Under this scheme, users will dot into arbitrary class members. This makes the interface consistent - since the verilog blackboxes don't have io.* prefixes on their IOs, Chisel users won't see those either.

The example is a stock example taken from the current BlackBox class, but refactored for the ExtModule API. I don't know why there are clocks there - that would also be an orthogonal fix.

Is there a strong demand for a BlackBox that automatically has io on all members?
That you use this case to justify the feature is bizarre.

The idea is that if there is no required .io and have multi top-level IO support, then you don't need to generate the io. prefix in the FIRRTL/Verilog.

sdtwigg · 2017-03-21T02:50:06Z

chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala

+    }
+
+    // All suggestions are in, force names to every node.
+    for (id <- _ids) {


This is a blackbox so it has no implementation. What nodes need ids?

Because the original BlackBox implementation had it. I think _onModuleClose is needed for Aggregates to do some kind of recursive naming / reference setting. I've added a comment there.

sdtwigg · 2017-03-21T03:16:26Z

chiselFrontend/src/main/scala/chisel3/core/Module.scala

+
+    override_clock match {
+      case Some(override_clock) => clock := override_clock
+      case _ => clock := Builder.forcedClock


What if the parent module can't resolve forcedClock?

Ditto, crash as would be the case if you create a Reg where you can't resolve forcedClock.

sdtwigg · 2017-03-21T03:16:50Z

chiselFrontend/src/main/scala/chisel3/core/Module.scala

-    this
+    override_reset match {
+      case Some(override_reset) => reset := override_reset
+      case _ => reset := Builder.forcedReset


What if the parent module can't resolve forcedReset?

sdtwigg · 2017-03-21T03:18:07Z

src/test/scala/chiselTests/BundleEltsSpec.scala

@@ -0,0 +1,84 @@
+// See LICENSE for license details.


Remove this test or fix it.

I'm not sure where this came from, probably git add'd something from another branch. Gone now.

sdtwigg · 2017-03-21T03:19:52Z

chiselFrontend/src/main/scala/chisel3/core/Module.scala

@@ -2,8 +2,12 @@



This file should be split into multiple files.

Split Module.scala (object Module, class BaseModule) and UserModule.scala (class UserModule, class ImplicitModule, class LegacyModule)

sdtwigg · 2017-03-21T03:20:31Z

chiselFrontend/src/main/scala/chisel3/core/Module.scala

}
+

What if the parent module cannot resolve forcedClock or forcedReset?

Then you get a crash, just like what would happen if you tried to instantiate a Reg inside a non-ImplicitModule.

A reg not being instantiated in a non-implicit module is a problem because there is no way to later supply that reg a clock. In this case, there is since the module already exposes clock and reset pins.

I would argue the stylistic way to do this is to use withClockAndReset, rather than assigning the clock and reset pins as connect statements.

Do you have a good example use case where you connect the clock and reset ports, but withClockAndReset is a bad fit?

ducky64

All comments addressed.
Major points:

some suggested code improvements are orthogonal to the module hierarchy refactor - I don't want this mega-PR to become even more mega. Can you open an issue so those can be discussed and addressed separately?
added tests for RawModule
Overall, if you instantiate an ImplicitModule inside a RawModule without a withClockAndReset block, it will fail. This is consistent with what would happen if you instantiated a Reg inside the same.
the benefits vs. drawbacks of allowing multiple top-level IO has been debated, and the resolution (which this implements) is to try allowing multiple IOs in experimental. The debate included whether it was appropriate to have reflection name IO signals and consistency between ExtModule and other regular module types.
refactoring to limit access to the internal mutable variables like _ids, _commands, _ports
other general (mostly style / comments) cleanup

ducky64 · 2017-03-21T19:44:37Z

chiselFrontend/src/main/scala/chisel3/core/BiConnect.scala

@@ -121,12 +121,12 @@ object BiConnect {

  // This function checks if element-level connection operation allowed.
  // Then it either issues it or throws the appropriate exception.
-  def elemConnect(implicit sourceInfo: SourceInfo, connectCompileOptions: CompileOptions, left: Element, right: Element, context_mod: Module): Unit = {
+  def elemConnect(implicit sourceInfo: SourceInfo, connectCompileOptions: CompileOptions, left: Element, right: Element, context_mod: UserModule): Unit = {


I think this code improvement is orthogonal to the module refactor, this can be done as a follow-on PR.

ducky64 · 2017-03-21T19:51:15Z

chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala

+  *   val O = IO(Output(Clock()))
+  *   val I = IO(Input(Clock()))
+  *   val IB = IO(Input(Clock()))
+  * }


ExtModule is an experimental API and aims for consistency with regular Module (where you have the io. prefix for io members; old BlackBox dropped the io. prefix when emitting Verilog).

Under this scheme, users will dot into arbitrary class members. This makes the interface consistent - since the verilog blackboxes don't have io.* prefixes on their IOs, Chisel users won't see those either.

The example is a stock example taken from the current BlackBox class, but refactored for the ExtModule API. I don't know why there are clocks there - that would also be an orthogonal fix.

ducky64 · 2017-03-21T19:54:30Z

chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala

+
+    val names = nameIds(classOf[ExtModule])
+
+    // Name ports based on reflection


But you get consistency in return, in that the io. prefix isn't being silently dropped.

Again, this is experimental, and we'll see whether people like this or the old style more. I don't think reflection has been a problem, and checks ensure that it will never silently drop a port.

ducky64 · 2017-03-21T19:59:25Z

chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala

+    }
+
+    // All suggestions are in, force names to every node.
+    for (id <- _ids) {


Because the original BlackBox implementation had it. I think _onModuleClose is needed for Aggregates to do some kind of recursive naming / reference setting. I've added a comment there.

ducky64 · 2017-03-21T20:00:47Z

chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala

+  private[core] def initializeInParent() {
+    implicit val sourceInfo = UnlocatableSourceInfo
+
+    _ports.foreach { x: Data =>


ducky64 · 2017-03-21T21:29:41Z

chiselFrontend/src/main/scala/chisel3/core/Module.scala

+    val names = super.nameIds(rootClass)
+
+    // Allow IO naming without reflection
+    names.put(io, "io")


The standard names get priority (HashMap updated after the reflection naming).

ducky64 · 2017-03-21T21:30:09Z

chiselFrontend/src/main/scala/chisel3/core/Module.scala

+
+    override_clock match {
+      case Some(override_clock) => clock := override_clock
+      case _ => clock := Builder.forcedClock


Ditto, crash as would be the case if you create a Reg where you can't resolve forcedClock.

ducky64 · 2017-03-21T21:30:16Z

chiselFrontend/src/main/scala/chisel3/core/Module.scala

-    this
+    override_reset match {
+      case Some(override_reset) => reset := override_reset
+      case _ => reset := Builder.forcedReset


ducky64 · 2017-03-21T21:35:31Z

src/main/scala/chisel3/compatibility.scala

@@ -152,16 +152,43 @@ package object Chisel {     // scalastyle:ignore package.object.name
  object Bool extends BoolFactory
  val Mux = chisel3.core.Mux

-  type BlackBox = chisel3.core.BlackBox
+  import chisel3.core.Param
+  abstract class BlackBox(params: Map[String, Param] = Map.empty[String, Param]) extends chisel3.core.BlackBox(params) {


Done (this auto wraps IO)

ducky64 · 2017-03-21T21:37:36Z

src/test/scala/chiselTests/BundleEltsSpec.scala

@@ -0,0 +1,84 @@
+// See LICENSE for license details.


I'm not sure where this came from, probably git add'd something from another branch. Gone now.

sdtwigg · 2017-03-21T22:31:28Z

I find these arguments to 'consistency' are weak, particularly because you are exposing very new functionality. Also, the argument is not compelling. I can similarly argue that it is inconsistent that ImplicitModule and BlackBox IO must be accessed wholly through io but RawModule and ExtModule io must access through arbitrary members.

Similarly, the user of a module must now comb through the implementation of RawModule and ExtModule to find which fields are actually ports. (This is even setting aside designers being 'clever' and parametrically making certain fields IOs versus internal nodes as demonstrated in the prior issue.)

ducky64 · 2017-03-21T23:11:37Z

With regards to the multi IO behavior, we can have another discussion about it during the next Chisel meeting, but the resolution from the last discussion was that it does have benefits and it's worth trying.

With regards to consistency: I'd argue that (assuming people like the Multi IO idea) the MultiIOModule and ExtModule style will be the new style.

jackkoenig · 2017-03-22T19:22:31Z

src/main/scala/chisel3/package.scala

+    type BaseModule = chisel3.core.BaseModule
+    type MultiIOModule = chisel3.core.ImplicitModule
+    type RawModule = chisel3.core.UserModule
+    type ExtModule = chisel3.core.ExtModule


Move to experimental

... this is in experimental ...
(GitHub diff apparently can't handle inner classes / objects / scopes when telling you the enclosing scope of a block of code)

jackkoenig · 2017-03-22T19:23:50Z

src/test/scala/chiselTests/ExtModule.scala

+
+// Avoid collisions with regular BlackBox tests by putting ExtModule blackboxes
+// in their own scope.
+package ExtModule {


Should this just be a new package inside of chiselTests including the directory structure?

I just need this to deconflict the scope, and the current implementation seems lightweight and fine.

jackkoenig · 2017-03-22T19:23:54Z

src/test/scala/chiselTests/ExtModule.scala

+import chisel3.experimental._
+import chisel3.testers.BasicTester
+import chisel3.util._
+//import chisel3.core.ExplicitCompileOptions.Strict


Remove comment

ducky64 · 2017-03-22T20:28:55Z

@sdtwigg @jackkoenig issues fixed, inheritance composition with multiple top level IOs test added. Turns out the new stuff was in experimental all along, just GitHub doesn't do a good job of showing inner enclosing scopes.

Martoni · 2017-03-30T13:40:13Z

Have you got a simple example to test this pull request ? I tried it with a simple blinking led project : https://github.com/Martoni/blp/tree/master/platforms/kintex7/chisel3

But I can't manage to generate verilog:
https://groups.google.com/forum/#!topic/chisel-users/m-ibJXZjm3E

ducky64 · 2017-03-30T20:22:59Z

@Martoni oversight in forgetting to refactor the Driver methods; fixed now.
In your example, you will also need to encapsulate your Module-inside-RawModule instantiation in a withClockAndReset block (needs to also be imported from chisel3.experimental).

sdtwigg · 2017-03-30T21:43:16Z

Heh, I made the same oversight internally.

Martoni · 2017-04-01T20:22:02Z

Thanks @ducky64, I pulled the last commits. But I can't find how to encapsulate my modules correctly in withClockAndReset. (How to do reset if I don't have one ?).

ducky64 · 2017-04-01T20:22:53Z

You can pass in any Bool for the reset signal, including false.B

Martoni · 2017-04-04T11:55:52Z

Hi,
Thanks @ducky64, I managed to compile my blink design :
https://github.com/Martoni/blp/tree/master/platforms/kintex7/chisel3

Works for me.

ducky64 · 2017-04-13T01:39:19Z

Rebased to latest master.

sdtwigg · 2017-04-13T01:40:19Z

This is probably fine as long as the controversial parts actually remain in experimental.

ducky64 · 2017-04-13T01:51:54Z

Hooray!
... waiting for tests ...

ducky64 · 2017-04-13T02:10:30Z

retest this please

Broke no longer broken Fix blackbox Moar fix Refactor mod_context -> UserModule Stricter checks on ImplicitModule MultiIO Module extmodule support MultiIOTester Expose RawModule

ucbjrl · 2017-04-13T19:55:23Z

retest this please

ucbjrl · 2017-04-14T02:48:57Z

retest this please

edwardcwang · 2017-04-14T06:07:32Z

Another car in the mega-PR train bites the dust?

ducky64 · 2017-04-14T06:16:10Z

That's actually it for outstanding mega-PRs right now, the rest of them are mostly around 100 lines changes. So the entire PR train is in the station ... for now ...

Martoni · 2017-04-27T07:46:43Z

That works for me :) Then I could respond to my stackoverflow question : http://stackoverflow.com/questions/41427717/how-to-delete-clock-signal-on-chisel3-top-module/43651603#43651603

azidar requested a review from sdtwigg February 1, 2017 20:33

ducky64 mentioned this pull request Feb 2, 2017

Module ports beyond io #101

Closed

jackkoenig added this to the 3.0.0 milestone Feb 22, 2017

ducky64 force-pushed the modhier branch from b499bb9 to 931ea9d Compare February 24, 2017 21:17

ducky64 requested a review from jackkoenig February 25, 2017 03:12

ducky64 mentioned this pull request Mar 18, 2017

Special Top Module class to rename/delete clock/reset #427

Closed

sdtwigg suggested changes Mar 21, 2017

View reviewed changes

ducky64 force-pushed the modhier branch from 0fe5775 to 5acd445 Compare March 21, 2017 19:50

ducky64 commented Mar 21, 2017

View reviewed changes

jackkoenig reviewed Mar 22, 2017

View reviewed changes

ducky64 force-pushed the modhier branch from f930cb9 to 8a34019 Compare March 30, 2017 20:20

ducky64 force-pushed the modhier branch from 8a34019 to aecb59b Compare April 13, 2017 01:39

sdtwigg approved these changes Apr 13, 2017

View reviewed changes

jackkoenig approved these changes Apr 13, 2017

View reviewed changes

ducky64 added 6 commits April 13, 2017 12:05

Module hierarchy refactor

b1a1b25

Broke no longer broken Fix blackbox Moar fix Refactor mod_context -> UserModule Stricter checks on ImplicitModule MultiIO Module extmodule support MultiIOTester Expose RawModule

Address review comments

62bd0e6

RawModule tests, fixes to allow RawModule to instantiate

398e252

Add MultiIO inheritance-composition test / example

3c31088

Remove comment

51cdfbe

Allow top-level RawModule

e6f405e

ducky64 force-pushed the modhier branch from aecb59b to e6f405e Compare April 13, 2017 19:05

ducky64 merged commit e07248b into master Apr 14, 2017

ducky64 deleted the modhier branch April 14, 2017 05:59

jackkoenig mentioned this pull request Apr 15, 2017

In Chisel._, Module and BlackBox No Longer Share io Interface #581

Closed

Martoni mentioned this pull request Aug 24, 2017

Documentation Needed for RawModule and MultiIOModule #678

Closed


		val names = nameIds(classOf[ExtModule])

		// Name ports based on reflection

Module Hierarchy Refactor #469

Module Hierarchy Refactor #469

Conversation

ducky64 commented Jan 31, 2017 • edited Loading

sdtwigg commented Jan 31, 2017

ducky64 commented Jan 31, 2017

sdtwigg commented Jan 31, 2017

sdtwigg commented Jan 31, 2017

ducky64 commented Jan 31, 2017

sdtwigg commented Jan 31, 2017

ducky64 commented Jan 31, 2017

sdtwigg commented Jan 31, 2017

ducky64 commented Jan 31, 2017

ducky64 commented Feb 25, 2017

ducky64 commented Feb 25, 2017

ducky64 commented Mar 21, 2017

sdtwigg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ducky64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdtwigg commented Mar 21, 2017

ducky64 commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ducky64 commented Mar 22, 2017

Martoni commented Mar 30, 2017

ducky64 commented Mar 30, 2017

sdtwigg commented Mar 30, 2017

Martoni commented Apr 1, 2017

ducky64 commented Apr 1, 2017

Martoni commented Apr 4, 2017

ducky64 commented Apr 13, 2017

sdtwigg commented Apr 13, 2017

ducky64 commented Apr 13, 2017

ducky64 commented Apr 13, 2017

ucbjrl commented Apr 13, 2017

ucbjrl commented Apr 14, 2017

edwardcwang commented Apr 14, 2017

ducky64 commented Jan 31, 2017 •

edited

Loading

Martoni commented Apr 27, 2017 •

edited

Loading