[Moore] Support four-valued integers in ConstantOp #7463
Conversation
fabianschuiki
force-pushed
the
fschuiki/moore-fvint-constants
branch
from
3e2efe9 to
8ff5ec9
Compare
| def FVIntAttr : Attr<CPred<"llvm::isa<FVIntegerAttr>($_self)">, | ||
| "arbitrary precision four-valued integer attribute"> { | ||
| let storageType = [{ circt::moore::FVIntegerAttr }]; | ||
| let returnType = [{ circt::FVInt }]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Why isn't it possible to just use the FVIntegerAttr above?
There was a problem hiding this comment.
If you use FVIntegerAttr directly in constant op as (ins FVIntegerAttr:$value), then calling constOp.getValue() will return the FVIntegerAttr. If you use FVIntAttr instead, you will get a getValue() accessor which returns FVInt, and a getValueAttr() accessor which returns the FVIntegerAttr.
I took the FVIntAttr pattern above from the builtin IntegerAttr: hw.constant has to use APIntAttr instead of Builtin_IntegerAttr for the same reason. I don't know why this is so obscure in MLIR. Ideally you'd have some way of marking attributes that wrap an underlying type like APInt or FVInt, and which you'd like to unwrap to that value by default.
There was a problem hiding this comment.
It should work if you add the following two lines to FVIntegerAttr:
let convertFromStorage = "$_self.getValue()";
let returnType = [{ circt::FVInt }];
AttrDef overrides those two from the Attr class it inherits.
There was a problem hiding this comment.
🥳 That works! Really cool, thanks 😃
lib/Dialect/Moore/MooreOps.cpp
Outdated
| if (value == FVInt(1, 0)) { | ||
| p << "false"; | ||
| } else if (value == FVInt(1, 1)) { | ||
| p << "true"; |
There was a problem hiding this comment.
Nit: Not sure if this special casing is worth it since we need to specify the type anyways here.
There was a problem hiding this comment.
Yeah good point. I think my initial motivation to add them is to avoid having single-bit 0 and 1 printed as 0 and -1. But instead we could also just not use negative notation for single-bit numbers. That would probably make more sense 😁
fabianschuiki
force-pushed
the
fschuiki/moore-fvint-constants
branch
from
ae03cf9 to
2ef8f23
Compare
Extend Moore's `ConstantOp` to use the new `FVIntegerAttr`, which now
allows us to materialize four-valued integer constants in the IR. Also
adjust ImportVerilog to finally properly captured integer literals with
X and Z bits.
With this change, `circt-verilog` is now capable of fully parsing the
Snitch RISC-V core used as a benchmark in the original LLHD paper at
PLDI 2020, and to produce a corresponding blob of IR.
Examples of the extended op:
moore.constant 0 : i32
moore.constant 2 : i2
moore.constant -2 : i2
moore.constant h123456789ABCDEF0 : i64
moore.constant h123456789ABCDEF0XZ : l72
moore.constant b1010 : i8
moore.constant b1010XZ : l8
fabianschuiki
force-pushed
the
fschuiki/moore-fvint-constants
branch
from
2ef8f23 to
add0b6a
Compare
| OpBuilder<(ins "IntType":$type, "const FVInt &":$value)>, | ||
| OpBuilder<(ins "IntType":$type, "const APInt &":$value)>, | ||
| OpBuilder<(ins "IntType":$type, "int64_t":$value)>, |
There was a problem hiding this comment.
From my perspective, I think FVInt can shoulder the creation of moore.constant. So should we still need APInt?
There was a problem hiding this comment.
I totally agree! The builder with APInt is just here for convenience now, it internally calls the builder with FVInt and converts the APInt to an FVInt without any X or Z bits.
Extend Moore's
ConstantOpto use the newFVIntegerAttr, which now allows us to materialize four-valued integer constants in the IR. Also adjust ImportVerilog to finally properly captured integer literals with X and Z bits.With this change,
circt-verilogis now capable of fully parsing the Snitch RISC-V core used as a benchmark in the original LLHD paper at PLDI 2020, and to produce a corresponding blob of IR.Examples of the extended op: