phanrahan / magmathon

Magma Hackathon
MIT License
11 stars 3 forks source link

Prep for Facebook Magmathon #22

Closed phanrahan closed 6 years ago

phanrahan commented 6 years ago
leonardt commented 6 years ago

I made a pass over the chisel-tutorial notebooks, they are in working order. We should add more text to them to explain things. I also renamed them to coreir-tutorial with the intention that they could be used to demonstrate using the coreir backend and simulator (don't need an icestick, so this can be done virtually). I thought this was more relevant than chisel-tutorial since that's just the source of the circuits (this is still documented in the README). This also fits better if we rename tutorial to icestick-tutorial since those depend on the icestick.

leonardt commented 6 years ago

Made a pass on tutorial notebooks, they are now in working order. I noticed some wonky behavior with the popcount notebook when testing it with my hardware setup. I'll investigate this further tomorrow, not sure if it's an issue with my hardware or the circuit.

leonardt commented 6 years ago

Reran the advanced notebooks, they seem to be in working order. There's an issue in fault that is blocking the verilator notebook, so we'll have to wait for that to be resolved before we can get it working again. I did not test the lut notebook with hardware, will do that tomorrow.

phanrahan commented 6 years ago

I remember Chick said that he couldn't get the popcount notebook to work. I can look into that.

leonardt commented 6 years ago

Okay, all notebooks have been run to verify that they don't crash due to magma/mantle changes. Still need to do any editing pass to add/clean up content. I think the tutorial notebooks look pretty good still, the advanced notebooks could use more documentation. Also I think it would be useful to add a notebook that introduces fault for testing.

phanrahan commented 6 years ago

@leonardt great! I have some time today and I will also take a look at them.

leonardt commented 6 years ago

Headers soldered on the 8 new icesticks. The joints passed visual inspection but I was paranoid so I went ahead and tested them using the j1j3.py program and using the 5v -> switch -> input and output -> led -> gnd setup. All the boards passed (basically pass through the switch value to the LED output) using both 3.3v and GND pins on each side. Success!

phanrahan commented 6 years ago

@leonardt Sounds great!

leonardt commented 6 years ago

With https://github.com/phanrahan/magma/commit/d8a3b7833e8777b2a2ad6183e1f2e7440bd66c95, wiring errors for the following circuit

    Buf = DeclareCircuit('Buf', "I", In(Array(8, Bit)), "O", Out(Array(8, Bit)))

    main = DefineCircuit("main", "I", In(Bit), "O", Out(Array(7, Bit)))

    buf = Buf()
    wire(main.O, buf.I)

look like

magma:ERROR:tests/test_type/test_type_errors.py:10: Cannot wire main.O (type=Array(7,In(Bit)), len=8) to inst0.I (type=Array(8,In(Bit)), len=7) because
the arrays do not have the same length
magma:ERROR:    wire(main.O, buf.I)

I'm going to work on improving the names generated (right now it just uses repr, but I'll start with Pat's suggestion for using the definition names)

phanrahan commented 6 years ago

I think main.Buf_inst0.I would be better.

leonardt commented 6 years ago

With https://github.com/phanrahan/magma/commit/6998248fc33ad3f95a2541b5c9fd5f5832c177b1, for the following circuit

    A = DeclareCircuit('A', "I", In(Bit), "O", Out(Bit))

    main = DefineCircuit("main", "I", In(Bit), "O", Out(Bit))

    a = A()
    wire(main.I, a.O)

we get the error message

tests/test_wire/test_errors.py:23: Using main.A_inst0.O (an output) as an input
    wire(main.I, a.O)

Notice the name main.A_inst0.O, still unforunate that we can't capture the a variable name yet, but it's better than before (which was just inst0.O).

leonardt commented 6 years ago

Okay here's the better name.

def test_array_lengths(caplog):
    Buf = DeclareCircuit('Buf', "I", In(Array(8, Bit)), "O", Out(Array(8, Bit)))

    main = DefineCircuit("main", "I", In(Bit), "O", Out(Array(7, Bit)))

    buf = Buf()
    wire(main.O, buf.I)

produces

tests/test_type/test_type_errors.py:10: Cannot wire main.O (type=Array(7,In(Bit)), len=8) to main.Buf_inst0.I (type=Array(8,In(Bit)), len=7) because the arrays do not have the same length
    wire(main.O, buf.I)
leonardt commented 6 years ago

Okay, I've combed through the code and moved any print statements still in action to use the logging modules.

Overall I think the error/warning/debug logging infrastructure is much improved. Raj and I were already benefiting from it today for the UART. I think there's still work to be done here, but we've definitely made great progress re: usability. I think the wiring errors/warnings are much more helpful now.

leonardt commented 6 years ago

Here's an example of passing filename/lineno to the verilog backend https://github.com/phanrahan/magma/blob/78d31d0b80ee4e04520459f31a36d19caa4f1250/tests/test_circuit/gold/test_for_loop_def.v

Users can do this using the API m.set_codegen_debug_info(True) or an environment variable MAGMA_CODEGEN_DEBUG_INFO=1

leonardt commented 6 years ago

(That latest version depends on the coreir-conn-metadata branch to be merged in. If I cna't get that in before tomorrow, depending on coreir changes, then I can merge the verilog changes and leave out the coreir changes).

leonardt commented 6 years ago

Closing this, good work all!