From 0e95d97f1f7980637dfe2fe3ee79410dfb16050f Mon Sep 17 00:00:00 2001 From: Chris Lattner Date: Wed, 8 Apr 2020 11:31:46 -0700 Subject: [PATCH] Teach the mlir asmprinter to use the name attribute (when present) to name the result SSA values, this makes the asmdumps waaay more readable. --- README.md | 7 +++-- lib/Dialect/FIRRTL/Dialect.cpp | 15 ++++++---- test/fir/basic.fir | 52 +++++++++++++++++----------------- test/fir/locations.fir | 12 ++++---- 4 files changed, 46 insertions(+), 40 deletions(-) diff --git a/README.md b/README.md index 713216db9510..1c46ac8265f9 100644 --- a/README.md +++ b/README.md @@ -37,13 +37,16 @@ reasonably fast and is good to provide a sanity check that things are working): $ cd ~/Projects/llvm-project $ mkdir build $ cd build -$ cmake -G Ninja ../llvm -DLLVM_EXTERNAL_PROJECTS="spt" -DLLVM_EXTERNAL_SPT_SOURCE_DIR=/Users/chrisl/Projects/spt -DLLVM_ENABLE_PROJECTS="mlir;spt" -DLLVM_TARGETS_TO_BUILD="X86;RISCV" -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON -DCMAKE_BUILD_TYPE=DEBUG +$ cmake -G Ninja ../llvm -DLLVM_EXTERNAL_PROJECTS="spt" -DLLVM_EXTERNAL_SPT_SOURCE_DIR=/Users/chrisl/Projects/spt -DLLVM_ENABLE_PROJECTS="mlir;spt" -DLLVM_TARGETS_TO_BUILD="X86;RISCV" -DLLVM_ENABLE_ASSERTIONS=ON -DCMAKE_BUILD_TYPE=DEBUG ``` -The "-DCMAKE_BUILD_TYPE=DEBUG" flag enables debug information, which makes the +The `-DCMAKE_BUILD_TYPE=DEBUG` flag enables debug information, which makes the whole tree compile slower, but allows you to step through code into the LLVM and MLIR frameworks. +To get something that runs fast, use `-DCMAKE_BUILD_TYPE=Release`. This can +make a very large difference in performance. + 5) Build MLIR and run MLIR tests as a smoketest: ``` diff --git a/lib/Dialect/FIRRTL/Dialect.cpp b/lib/Dialect/FIRRTL/Dialect.cpp index 9ddc55403508..de4add85e3ec 100644 --- a/lib/Dialect/FIRRTL/Dialect.cpp +++ b/lib/Dialect/FIRRTL/Dialect.cpp @@ -16,8 +16,11 @@ using namespace firrtl; //===----------------------------------------------------------------------===// // If the specified attribute set contains the firrtl.name attribute, return it. -static StringAttr getFIRRTLNameAttr(ArrayRef attrs) { +static StringAttr getModuleFIRRTLNameAttr(ArrayRef attrs) { for (auto &argAttr : attrs) { + // FIXME: We currently use firrtl.name instead of name because this makes + // the FunctionLike handling in MLIR core happier. It otherwise doesn't + // allow attributes on module parameters. if (!argAttr.first.is("firrtl.name")) continue; @@ -30,7 +33,7 @@ static StringAttr getFIRRTLNameAttr(ArrayRef attrs) { namespace { // We implement the OpAsmDialectInterface so that FIRRTL dialect operations -// automatically interpret the firrtl.name attribute on function arguments and +// automatically interpret the name attribute on function arguments and // on operations as their SSA name. struct FIRRTLOpAsmDialectInterface : public OpAsmDialectInterface { using OpAsmDialectInterface::OpAsmDialectInterface; @@ -40,7 +43,7 @@ struct FIRRTLOpAsmDialectInterface : public OpAsmDialectInterface { void getAsmResultNames(Operation *op, OpAsmSetValueNameFn setNameFn) const override { if (op->getNumResults() > 0) - if (auto nameAttr = op->getAttrOfType("firrtl.name")) + if (auto nameAttr = op->getAttrOfType("name")) setNameFn(op->getResult(0), nameAttr.getValue()); } @@ -54,7 +57,7 @@ struct FIRRTLOpAsmDialectInterface : public OpAsmDialectInterface { for (size_t i = 0, e = block->getNumArguments(); i != e; ++i) { // Scan for a 'firrtl.name' attribute. - if (auto str = getFIRRTLNameAttr(impl::getArgAttrs(parentOp, i))) + if (auto str = getModuleFIRRTLNameAttr(impl::getArgAttrs(parentOp, i))) setNameFn(block->getArgument(i), str.getValue()); } } @@ -150,7 +153,7 @@ void firrtl::getModulePortInfo(Operation *op, for (unsigned i = 0, e = argTypes.size(); i < e; ++i) { auto argAttrs = ::mlir::impl::getArgAttrs(op, i); results.push_back( - {getFIRRTLNameAttr(argAttrs), argTypes[i].cast()}); + {getModuleFIRRTLNameAttr(argAttrs), argTypes[i].cast()}); } } @@ -238,7 +241,7 @@ static void printFunctionSignature2(OpAsmPrinter &p, Operation *op, ArrayRef elidedAttrs; StringRef tmp; if (argumentValue) { - if (auto nameAttr = getFIRRTLNameAttr(argAttrs)) { + if (auto nameAttr = getModuleFIRRTLNameAttr(argAttrs)) { // Check to make sure the asmprinter is printing it correctly. SmallString<32> resultNameStr; diff --git a/test/fir/basic.fir b/test/fir/basic.fir index 4c7c744a7f22..fe6a6d3cb482 100644 --- a/test/fir/basic.fir +++ b/test/fir/basic.fir @@ -49,19 +49,19 @@ circuit basic : ; CHECK: firrtl.circuit "basic" { output auto : UInt<1> ; CHECK: %auto: !firrtl.flip>, input i8 : UInt<8> ; CHECK: %i8: !firrtl.uint<8>) - ; CHECK: %0 = firrtl.wire {name = "_T"} : !firrtl.vector, 12> + ; CHECK: %_T = firrtl.wire {name = "_T"} : !firrtl.vector, 12> wire _T : UInt<1>[12] @[Nodes.scala 370:76] - ; CHECK: %1 = firrtl.wire {name = "_T_2"} : !firrtl.vector, 13> + ; CHECK: %_T_2 = firrtl.wire {name = "_T_2"} : !firrtl.vector, 13> wire _T_2 : UInt<1>[13] - ; CHECK: %2 = firrtl.wire {name = "out_0"} : !firrtl.bundle>>> + ; CHECK: %out_0 = firrtl.wire {name = "out_0"} : !firrtl.bundle>>> wire out_0 : { member : { 0 : { clock : Clock, reset : UInt<1>}}} - ; CHECK: firrtl.connect %0, %1 : !firrtl.vector, 12>, !firrtl.vector, 13> + ; CHECK: firrtl.connect %_T, %_T_2 : !firrtl.vector, 12>, !firrtl.vector, 13> _T <= _T_2 - ; CHECK: firrtl.partialconnect %0, %1 + ; CHECK: firrtl.partialconnect %_T, %_T_2 _T <- _T_2 ; CHECK: firrtl.invalid %auto : !firrtl.flip> @@ -73,18 +73,18 @@ circuit basic : ; CHECK: firrtl.circuit "basic" { ; CHECK: firrtl.invalid %reset : !firrtl.uint<1> reset is invalid - ; CHECK: %3 = firrtl.subfield %2("member") : (!firrtl.bundle>>>) -> !firrtl.bundle<0: bundle>> - ; CHECK: %4 = firrtl.subfield %3("0") : (!firrtl.bundle<0: bundle>>) -> !firrtl.bundle> - ; CHECK: %5 = firrtl.subfield %4("reset") : (!firrtl.bundle>) -> !firrtl.uint<1> - ; CHECK: firrtl.partialconnect %auto, %5 : !firrtl.flip>, !firrtl.uint<1> + ; CHECK: %0 = firrtl.subfield %out_0("member") : (!firrtl.bundle>>>) -> !firrtl.bundle<0: bundle>> + ; CHECK: %1 = firrtl.subfield %0("0") : (!firrtl.bundle<0: bundle>>) -> !firrtl.bundle> + ; CHECK: %2 = firrtl.subfield %1("reset") : (!firrtl.bundle>) -> !firrtl.uint<1> + ; CHECK: firrtl.partialconnect %auto, %2 : !firrtl.flip>, !firrtl.uint<1> auto <- out_0.member.0.reset @[Field 173:49] - ; CHECK: %6 = firrtl.subindex %1[0] : (!firrtl.vector, 13>) -> !firrtl.uint<1> - ; CHECK: %7 = firrtl.subindex %0[0] : (!firrtl.vector, 12>) -> !firrtl.uint<1> - ; CHECK: firrtl.connect %6, %7 + ; CHECK: %3 = firrtl.subindex %_T_2[0] : (!firrtl.vector, 13>) -> !firrtl.uint<1> + ; CHECK: %4 = firrtl.subindex %_T[0] : (!firrtl.vector, 12>) -> !firrtl.uint<1> + ; CHECK: firrtl.connect %3, %4 _T_2[0] <= _T[0] @[Xbar.scala 21:44] - ; CHECK: %8 = firrtl.node %auto {name = "n1"} : !firrtl.flip> + ; CHECK: %n1 = firrtl.node %auto {name = "n1"} : !firrtl.flip> node n1 = auto ; CHECK: firrtl.add %reset, %reset : (!firrtl.uint<1>, !firrtl.uint<1>) -> !firrtl.uint<2> @@ -100,18 +100,18 @@ circuit basic : ; CHECK: firrtl.circuit "basic" { auto <= add(UInt<10>(42), UInt<8>("hAB")) ; CHECK: firrtl.when %reset { - ; CHECK: firrtl.connect %0, %1 + ; CHECK: firrtl.connect %_T, %_T_2 ; CHECK: } else { - ; CHECK: firrtl.partialconnect %0 + ; CHECK: firrtl.partialconnect %_T, %_T_2 ; CHECK: } when reset : _T <= _T_2 else : _T <- _T_2 ; CHECK: firrtl.when %reset { - ; CHECK: [[N4A:%.+]] = firrtl.node %1 - ; CHECK: firrtl.connect %0, [[N4A]] + ; CHECK: [[N4A:%.+]] = firrtl.node %_T_2 + ; CHECK: firrtl.connect %_T, [[N4A]] ; CHECK: } else { - ; CHECK: [[N4B:%.+]] = firrtl.node %1 - ; CHECK: firrtl.partialconnect %0, [[N4B]] + ; CHECK: [[N4B:%.+]] = firrtl.node %_T_2 + ; CHECK: firrtl.partialconnect %_T, [[N4B]] ; CHECK: } when reset : node n4 = _T_2 @@ -123,13 +123,13 @@ circuit basic : ; CHECK: firrtl.circuit "basic" { ; CHECK: [[TMP:%.+]] = firrtl.constant 4 ; CHECK: [[COND:%.+]] = firrtl.lt %reset, [[TMP]] ; CHECK: firrtl.when [[COND]] { - ; CHECK: firrtl.connect %0, %1 + ; CHECK: firrtl.connect %_T, %_T_2 ; CHECK: } ; CHECK-NOT: else when lt(reset, UInt(4)) : ;; When with no else. _T <= _T_2 - ; CHECK: firrtl.printf %clock, %reset, "\22Something interesting! %x %x\22" %0, %1 : !firrtl.vector, 12>, !firrtl.vector, 13> + ; CHECK: firrtl.printf %clock, %reset, "\22Something interesting! %x %x\22" %_T, %_T_2 : !firrtl.vector, 12>, !firrtl.vector, 13> printf(clock, reset, "Something interesting! %x %x", _T, _T_2) ; CHECK: firrtl.stop %clock, %reset, 42 @@ -142,20 +142,20 @@ circuit basic : ; CHECK: firrtl.circuit "basic" { ; CHECK: firrtl.shr %i8, 8 : (!firrtl.uint<8>) -> !firrtl.uint<1> node n5 = or(shl(i8, 4), shr(i8, 8)) - ; CHECK: firrtl.dshl %i8, %24 : (!firrtl.uint<8>, !firrtl.uint<4>) -> !firrtl.uint<23> + ; CHECK: firrtl.dshl %i8, %{{.*}} : (!firrtl.uint<8>, !firrtl.uint<4>) -> !firrtl.uint<23> node n6 = dshl(i8, UInt<4>(7)) - ; CHECK: firrtl.cat %23, %26 : (!firrtl.uint<12>, !firrtl.uint<23>) -> !firrtl.uint<35> + ; CHECK: firrtl.cat %{{.*}}, %{{.*}} : (!firrtl.uint<12>, !firrtl.uint<23>) -> !firrtl.uint<35> node n7 = cat(n5, n6) - ; CHECK: firrtl.mux(%reset, %i8, %29) : (!firrtl.uint<1>, !firrtl.uint<8>, !firrtl.uint) -> !firrtl.uint + ; CHECK: firrtl.mux(%reset, %i8, %{{.*}}) : (!firrtl.uint<1>, !firrtl.uint<8>, !firrtl.uint) -> !firrtl.uint node n8 = mux(reset, i8, UInt(4)) - ; CHECK: firrtl.reginit %clock, %reset, %32 {name = "_T_2621"} : (!firrtl.clock, !firrtl.uint<1>, !firrtl.uint<4>) -> !firrtl.uint<4> + ; CHECK: firrtl.reginit %clock, %reset, %{{.*}} {name = "_T_2621"} : (!firrtl.clock, !firrtl.uint<1>, !firrtl.uint<4>) -> !firrtl.uint<4> reg _T_2621 : UInt<4>, clock with : reset => (reset, UInt<4>("h0")) @[Edges.scala 230:27] - ; CHECK: firrtl.div %i8, %34 : (!firrtl.uint<8>, !firrtl.uint<4>) -> !firrtl.uint<8> + ; CHECK: firrtl.div %i8, %{{.*}} : (!firrtl.uint<8>, !firrtl.uint<4>) -> !firrtl.uint<8> node n9 = div(i8, UInt<4>(4)) ; CHECK: firrtl.tail %i8 7 : (!firrtl.uint<8>) -> !firrtl.uint<1> diff --git a/test/fir/locations.fir b/test/fir/locations.fir index 51566bc3a267..b6bb9fd6c463 100644 --- a/test/fir/locations.fir +++ b/test/fir/locations.fir @@ -32,14 +32,14 @@ circuit basic : @[CIRCUIT.scala 127] input in: UInt output auto : { out_0 : UInt<1> } - ; CHECK: %0 = firrtl.wire {name = "out_0"} + ; CHECK: %out_0 = firrtl.wire {name = "out_0"} wire out_0 : { member : { 0 : { reset : UInt<1>}}} - ; %1 = firrtl.subfield %auto "out_0" : {{.*}} loc("Field":173:49) - ; %2 = firrtl.subfield %0 "member" : {{.*}} loc("Field":173:49) - ; %3 = firrtl.subfield %2 "0" : {{.*}} loc("Field":173:49) - ; %4 = firrtl.subfield %3 "reset" : {{.*}} loc("Field":173:49) - ; firrtl.partialconnect %1, %4 : {{.*}} loc("Field":173:49) + ; %0 = firrtl.subfield %auto "out_0" : {{.*}} loc("Field":173:49) + ; %1 = firrtl.subfield %out_0 "member" : {{.*}} loc("Field":173:49) + ; %2 = firrtl.subfield %1 "0" : {{.*}} loc("Field":173:49) + ; %3 = firrtl.subfield %2 "reset" : {{.*}} loc("Field":173:49) + ; firrtl.partialconnect %0, %3 : {{.*}} loc("Field":173:49) auto.out_0 <- out_0.member.0.reset @[Field 173:49] ; CIRCUIT: CHECK: } loc("CIRCUIT.scala":127:0)