[arc][LowerToLLVM] Handle i0 types more rigorously#10393
[arc][LowerToLLVM] Handle i0 types more rigorously#10393
Conversation
i0 is legal in MLIR but illegal in LLVM-IR. llvm#8871 added support for eliding most i0 uses in generated LLVM-IR. However there are ways i0s can sneak through the cracks - for example in function signatures. This occurs when ArcInline chooses not to inline functions that take i0 operands (which typically come with / degenerate from !hw.array<1xT> types). Mark i0 as illegal. Transform it to i1 for expediency. Driveby fix incorrect LLVM lowering in HWToLLVM (using op.getIndex() rather than adaptor.getIndex()). Marking i0 as illegal does also make the `llvm.mlir.constant(0 : i0) : i0` that were inserted earlier illegal too, so we do have to ensure that the hw::ConstantOp lowering correctly changes its attribute type as well as its result type. This is a bit grim, and perhaps points to a better fix being a dedicated "remove i0 and array<1xT>" pass.
jmolloy
requested review from
fabianschuiki,
fzi-hielscher and
maerhart
and removed request for
fabianschuiki and
fzi-hielscher
fabianschuiki
left a comment
fabianschuiki
left a comment
There was a problem hiding this comment.
Nice catch! I'm wondering whether this also covers stuff like comb.add ... : i0, or comb.xor .... : i0, and any other op that operates on i0. I like your idea of setting up a pass to remove i0, since that probably involves a lot of type rewriting (removing i0 fields from structs, rewriting N x i0 arrays to null arrays, etc.). This might be an opportunity to remove 0 x ... arrays, too. WDYT? I'm happy with landing a fix for a specific issue like you have it here, but if this points at a more fundamental issue that a workaround isn't going to reliably address, we might want to have a deliberate i0-removal pass.
|
The changes LGTM, but echoing what @fabianschuiki wrote and quoting myself from #9733:
It would be nice to have a clear cut-off point in the pipeline beyond which i0 is no longer permitted. There already is a similar pass in |
* [Comb][AssumeTwoValued] Set twoState=true on all Comb ops This enables a bunch of Comb canonicalizations; see #10393. * Act on review comments, removing intermediate `using` lines --------- Co-authored-by: jmolloy@google.com <jmolloy@google.com>
i0 types are degenerate and come about as part of the addressing for
!hw.array<1xT>.
They are harmless except that LLVM-IR disallows them. Instead of our
current ad-hoc solutions for dealing with i0, nuke them in one swift pass.
This pass does a DialectConversion such that:
* i0 -> !opaque<"arc.poison">.
* !hw.array<1xT> -> T
* all composite types are converted recursively, e.g.
struct<x: array<1xi2>> -> struct<x: i2>
* All func arguments/results that are i0 are removed and replaced
with poison.
* All ops returning i0 that aren't otherwise handled are erased.
* All other ops have their results type-converted.
At the end we DCE to clean up, and there should be no more i0!
Assisted-by: Gemini:Experimental - added CHECK: lines and added more testcases.
|
Thanks folks! Your wish is my command, PTAL at RemoveI0TypesPass. The idea is that it can run reasonably late in our pipeline, when we only have arc.model and func.func as top-level ops. This reduces the amount of signature conversion that needs to be done and removes hw.module-like ops entirely from the equation. Still TODO (ran out of brainpower today):
|
|
This is really cool! |
This reverts commit f92aab8. (superseded)
* Remove existing ad-hoc i0 handling in LowerArcToLLVM. * Remove test for i0 in LowerArcToLLVM (i0 should now not exist on entry to this pass).
3 participants
i0 is legal in MLIR but illegal in LLVM-IR. #8871
added support for eliding most i0 uses in generated LLVM-IR.
However there are ways i0s can sneak through the cracks - for example in function
signatures. This occurs when ArcInline chooses not to inline functions that take
i0 operands (which typically come with / degenerate from !hw.array<1xT> types).
Mark i0 as illegal. Transform it to i1 for expediency. Driveby fix incorrect LLVM
lowering in HWToLLVM (using op.getIndex() rather than adaptor.getIndex()).
Marking i0 as illegal does also make the
llvm.mlir.constant(0 : i0) : i0that wereinserted earlier illegal too, so we do have to ensure that the hw::ConstantOp lowering
correctly changes its attribute type as well as its result type. This is a bit grim,
and perhaps points to a better fix being a dedicated "remove i0 and array<1xT>" pass.