Hi,
we are in the midst of an interesting work that begun with setting 'guessInstructionProperties = 0' in the SystemZ backend. We have found this to be useful, and discovered many instructions where the hasSideEffects flag was incorrectly set while it actually shouldn't.
The attached patch and test case triggers an assert in TwoAddress. (bin/llc ./tc_TwoAddr_crash.ll -mtriple=s390x-linux-gnu -mcpu=z13)
The only change in the patch is to remove the side effects flag from one instruction:
- def RISBMux : RotateSelectRIEfPseudo<GRX32, GRX32>;
+ let hasSideEffects = 0 in
+ def RISBMux : RotateSelectRIEfPseudo<GRX32, GRX32>;
The input to TwoAddress is:
BB#0: derived from LLVM BB %0
Live Ins: %r2l
%vreg0<def> = COPY %r2l<kill>; GR32Bit:%vreg0
%vreg9<def,tied1> = NIFMux %vreg0<tied0>, 14, %cc<imp-def,dead>; GRX32Bit:%vreg9 GR32Bit:%vreg0
%vreg4<def,tied1> = NIFMux %vreg0<tied0>, 254, %cc<imp-def,dead>; GRX32Bit:%vreg4 GR32Bit:%vreg0
%vreg2<def> = COPY %vreg0<kill>; GR32Bit:%vreg2,%vreg0
%vreg3<def> = COPY %vreg9<kill>; GR32Bit:%vreg3 GRX32Bit:%vreg9
\.\.\.
The NIFMux instructions will be converted by the SystemZ backend into RISBMux instructions. The tied source operand of the RISBMux is 0 (no-register / undef). This seems necessary as this is after 'Process implicit defs' pass.
The big change with the patch is that TwoAddress is now able to reschedule the RISBMux close to the killing instruction:
1: MBB->dump() = BB#0: derived from LLVM BB %0
Live Ins: %r2l
%vreg0<def> = COPY %r2l<kill>; GR32Bit:%vreg0
%vreg4<def,tied1> = NIFMux %vreg0<tied0>, 254, %cc<imp-def,dead>; GRX32Bit:%vreg4 GR32Bit:%vreg0
%vreg2<def> = COPY %vreg0; GR32Bit:%vreg2,%vreg0
%vreg9<def,tied1> = RISBMux %noreg<tied0>, %vreg0<kill>, 28, 158, 0; GRX32Bit:%vreg9 GR32Bit:%vreg0
%vreg3<def> = COPY %vreg9<kill>; GR32Bit:%vreg3 GRX32Bit:%vreg9
\.\.\.
This however means that TwoAddress will encounter this instruction once more as it iterates through MBB. That's when the assert triggers:
assert(SrcReg && SrcMO.isUse() && "two address instruction invalid");
It seems to me that since the %noreg makes sense (per above), and TwoAddress wants to schedule this instruction down the block, this assert is wrong here.
Should TwoAddress have a set of rescheduled instructions and not revisit them while iterating over MBB? Or is target allowed to actually build new 2-addr instructions here (seems odd)?
One alternative might be to first make a set of original instructions and only process them.
/Jonas
RISBMux_noSEFlag.patch (728 Bytes)
tc_TwoAddr_crash.ll (486 Bytes)