[llvm-dev] TwoAddressInstructionPass bug?

[Quentin Colombet via llvm-dev]

Hi Jonas,

Thanks for bringing that up.

> On Nov 30, 2017, at 4:04 AM, Jonas Paulsson via llvm-dev <llvm-dev at lists.llvm.org> wrote:
> 
> 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.

I agree.

> 
> 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)?

Having a set of rescheduled instructions and skipping them sounds reasonable to me.

Cheers,
-Quentin
> 
> One alternative might be to first make a set of original instructions and only process them.
> 
> /Jonas
> 
> 
> 
> <RISBMux_noSEFlag.patch><tc_TwoAddr_crash.ll>_______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev