<div dir="ltr"><div><div><div><div><div><div><div>Hi Ben,<br><br></div>thanks for committing this fix. You're absolutely right - the code was incorrect as the ANDIo does not materialize a new constant.<br></div>I know it seems like this transform is useless because it doesn't get rid of any instructions. However, the CR-setting rotate instructions are cracked instructions with double the total latency of ANDIo.<br><br></div>I'll post a follow-up on Phabricator. I think the complete fix should be:<br></div>- If the defining load-immediate has a single use, just update the immediate operand of the LI/LI8<br></div><div>- If the GPR result isn't needed, give the ANDIo an immediate that is either zero or the original immediate (to ensure the correct CR-bit is set)<br></div>- Otherwise (and in the late pass) use your fix <br><br></div>Thanks again and sorry about the issues this caused,<br></div>Nemanja<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 12, 2018 at 4:07 PM, Benjamin Kramer <span dir="ltr"><<a href="mailto:benny.kra@gmail.com" target="_blank">benny.kra@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Nemanja, can you review this? I didn't want to disable the entire<br>
transform again, but still avoid a miscompile that's triggered by it.<br>
<br>
On Fri, Jan 12, 2018 at 4:03 PM, Benjamin Kramer via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: d0k<br>
> Date: Fri Jan 12 07:03:24 2018<br>
> New Revision: 322373<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=322373&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=322373&view=rev</a><br>
> Log:<br>
> [PowerPC] Don't miscompile rotate+mask into an ANDIo if it can't recreate the immediate<br>
><br>
> I'm not even sure if this transform is ever worth it, but this at least<br>
> stops the bleeding.<br>
><br>
> Modified:<br>
> llvm/trunk/lib/Target/PowerPC/<wbr>PPCInstrInfo.cpp<br>
> llvm/trunk/test/CodeGen/<wbr>PowerPC/convert-rr-to-ri-<wbr>instrs.mir<br>
><br>
> Modified: llvm/trunk/lib/Target/PowerPC/<wbr>PPCInstrInfo.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.cpp?rev=322373&r1=322372&r2=322373&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/Target/<wbr>PowerPC/PPCInstrInfo.cpp?rev=<wbr>322373&r1=322372&r2=322373&<wbr>view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/lib/Target/PowerPC/<wbr>PPCInstrInfo.cpp (original)<br>
> +++ llvm/trunk/lib/Target/PowerPC/<wbr>PPCInstrInfo.cpp Fri Jan 12 07:03:24 2018<br>
> @@ -2444,6 +2444,8 @@ bool PPCInstrInfo::<wbr>convertToImmediateFor<br>
> Is64BitLI = Opc != PPC::RLDICL_32;<br>
> NewImm = InVal.getSExtValue();<br>
> SetCR = Opc == PPC::RLDICLo;<br>
> + if (SetCR && (SExtImm & NewImm) != NewImm)<br>
> + return false;<br>
> break;<br>
> }<br>
> return false;<br>
> @@ -2471,6 +2473,8 @@ bool PPCInstrInfo::<wbr>convertToImmediateFor<br>
> Is64BitLI = Opc == PPC::RLWINM8 || Opc == PPC::RLWINM8o;<br>
> NewImm = InVal.getSExtValue();<br>
> SetCR = Opc == PPC::RLWINMo || Opc == PPC::RLWINM8o;<br>
> + if (SetCR && (SExtImm & NewImm) != NewImm)<br>
> + return false;<br>
> break;<br>
> }<br>
> return false;<br>
><br>
> Modified: llvm/trunk/test/CodeGen/<wbr>PowerPC/convert-rr-to-ri-<wbr>instrs.mir<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/PowerPC/convert-rr-to-ri-instrs.mir?rev=322373&r1=322372&r2=322373&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/<wbr>CodeGen/PowerPC/convert-rr-to-<wbr>ri-instrs.mir?rev=322373&r1=<wbr>322372&r2=322373&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/test/CodeGen/<wbr>PowerPC/convert-rr-to-ri-<wbr>instrs.mir (original)<br>
> +++ llvm/trunk/test/CodeGen/<wbr>PowerPC/convert-rr-to-ri-<wbr>instrs.mir Fri Jan 12 07:03:24 2018<br>
> @@ -561,6 +561,16 @@<br>
> }<br>
><br>
> ; Function Attrs: norecurse nounwind readnone<br>
> + define i64 @testRLDICLo2(i64 %a, i64 %b) local_unnamed_addr #0 {<br>
> + entry:<br>
> + %shr = lshr i64 %a, 11<br>
> + %and = and i64 %shr, 16777215<br>
> + %tobool = icmp eq i64 %and, 0<br>
> + %cond = select i1 %tobool, i64 %b, i64 %and<br>
> + ret i64 %cond<br>
> + }<br>
> +<br>
> + ; Function Attrs: norecurse nounwind readnone<br>
> define zeroext i32 @testRLWINM(i32 zeroext %a) local_unnamed_addr #0 {<br>
> entry:<br>
> %shl = shl i32 %a, 4<br>
> @@ -602,6 +612,15 @@<br>
> }<br>
><br>
> ; Function Attrs: norecurse nounwind readnone<br>
> + define zeroext i32 @testRLWINMo2(i32 zeroext %a, i32 zeroext %b) local_unnamed_addr #0 {<br>
> + entry:<br>
> + %and = and i32 %a, 255<br>
> + %tobool = icmp eq i32 %and, 0<br>
> + %cond = select i1 %tobool, i32 %b, i32 %a<br>
> + ret i32 %cond<br>
> + }<br>
> +<br>
> + ; Function Attrs: norecurse nounwind readnone<br>
> define i64 @testRLWINM8o(i64 %a, i64 %b) local_unnamed_addr #0 {<br>
> entry:<br>
> %<a href="http://a.tr" rel="noreferrer" target="_blank">a.tr</a> = trunc i64 %a to i32<br>
> @@ -3904,6 +3923,59 @@ body: |<br>
><br>
> ...<br>
> ---<br>
> +name: testRLDICLo2<br>
> +# CHECK-ALL: name: testRLDICLo2<br>
> +alignment: 4<br>
> +exposesReturnsTwice: false<br>
> +legalized: false<br>
> +regBankSelected: false<br>
> +selected: false<br>
> +tracksRegLiveness: true<br>
> +registers:<br>
> + - { id: 0, class: g8rc, preferred-register: '' }<br>
> + - { id: 1, class: g8rc_and_g8rc_nox0, preferred-register: '' }<br>
> + - { id: 2, class: g8rc_and_g8rc_nox0, preferred-register: '' }<br>
> + - { id: 3, class: crrc, preferred-register: '' }<br>
> + - { id: 4, class: g8rc, preferred-register: '' }<br>
> +liveins:<br>
> + - { reg: '%x3', virtual-reg: '%0' }<br>
> + - { reg: '%x4', virtual-reg: '%1' }<br>
> +frameInfo:<br>
> + isFrameAddressTaken: false<br>
> + isReturnAddressTaken: false<br>
> + hasStackMap: false<br>
> + hasPatchPoint: false<br>
> + stackSize: 0<br>
> + offsetAdjustment: 0<br>
> + maxAlignment: 0<br>
> + adjustsStack: false<br>
> + hasCalls: false<br>
> + stackProtector: ''<br>
> + maxCallFrameSize: 4294967295<br>
> + hasOpaqueSPAdjustment: false<br>
> + hasVAStart: false<br>
> + hasMustTailInVarArgFunc: false<br>
> + savePoint: ''<br>
> + restorePoint: ''<br>
> +fixedStack:<br>
> +stack:<br>
> +constants:<br>
> +body: |<br>
> + bb.0.entry:<br>
> + liveins: %x3, %x4<br>
> +<br>
> + %1 = COPY %x4<br>
> + %0 = LI8 200<br>
> + %2 = RLDICLo %0, 61, 3, implicit-def %cr0<br>
> + ; CHECK-NOT: ANDI<br>
> + ; CHECK-LATE-NOT: andi.<br>
> + %3 = COPY killed %cr0<br>
> + %4 = ISEL8 %1, %2, %3.sub_eq<br>
> + %x3 = COPY %4<br>
> + BLR8 implicit %lr8, implicit %rm, implicit %x3<br>
> +<br>
> +...<br>
> +---<br>
> name: testRLWINM<br>
> # CHECK-ALL: name: testRLWINM<br>
> alignment: 4<br>
> @@ -4163,6 +4235,69 @@ body: |<br>
> %5 = COPY killed %cr0<br>
> %6 = ISEL %2, %3, %5.sub_eq<br>
> %8 = IMPLICIT_DEF<br>
> + %7 = INSERT_SUBREG %8, killed %6, 1<br>
> + %9 = RLDICL killed %7, 0, 32<br>
> + %x3 = COPY %9<br>
> + BLR8 implicit %lr8, implicit %rm, implicit %x3<br>
> +<br>
> +...<br>
> +---<br>
> +name: testRLWINMo2<br>
> +# CHECK-ALL: name: testRLWINMo2<br>
> +alignment: 4<br>
> +exposesReturnsTwice: false<br>
> +legalized: false<br>
> +regBankSelected: false<br>
> +selected: false<br>
> +tracksRegLiveness: true<br>
> +registers:<br>
> + - { id: 0, class: g8rc, preferred-register: '' }<br>
> + - { id: 1, class: g8rc, preferred-register: '' }<br>
> + - { id: 2, class: gprc_and_gprc_nor0, preferred-register: '' }<br>
> + - { id: 3, class: gprc_and_gprc_nor0, preferred-register: '' }<br>
> + - { id: 4, class: gprc, preferred-register: '' }<br>
> + - { id: 5, class: crrc, preferred-register: '' }<br>
> + - { id: 6, class: gprc, preferred-register: '' }<br>
> + - { id: 7, class: g8rc, preferred-register: '' }<br>
> + - { id: 8, class: g8rc, preferred-register: '' }<br>
> + - { id: 9, class: g8rc, preferred-register: '' }<br>
> +liveins:<br>
> + - { reg: '%x3', virtual-reg: '%0' }<br>
> + - { reg: '%x4', virtual-reg: '%1' }<br>
> +frameInfo:<br>
> + isFrameAddressTaken: false<br>
> + isReturnAddressTaken: false<br>
> + hasStackMap: false<br>
> + hasPatchPoint: false<br>
> + stackSize: 0<br>
> + offsetAdjustment: 0<br>
> + maxAlignment: 0<br>
> + adjustsStack: false<br>
> + hasCalls: false<br>
> + stackProtector: ''<br>
> + maxCallFrameSize: 4294967295<br>
> + hasOpaqueSPAdjustment: false<br>
> + hasVAStart: false<br>
> + hasMustTailInVarArgFunc: false<br>
> + savePoint: ''<br>
> + restorePoint: ''<br>
> +fixedStack:<br>
> +stack:<br>
> +constants:<br>
> +body: |<br>
> + bb.0.entry:<br>
> + liveins: %x3, %x4<br>
> +<br>
> + %1 = COPY %x4<br>
> + %0 = COPY %x3<br>
> + %2 = COPY %1.sub_32<br>
> + %3 = LI -22<br>
> + %4 = RLWINMo %3, 5, 24, 31, implicit-def %cr0<br>
> + ; CHECK-NOT: ANDI<br>
> + ; CHECK-LATE-NOT: andi.<br>
> + %5 = COPY killed %cr0<br>
> + %6 = ISEL %2, %3, %5.sub_eq<br>
> + %8 = IMPLICIT_DEF<br>
> %7 = INSERT_SUBREG %8, killed %6, 1<br>
> %9 = RLDICL killed %7, 0, 32<br>
> %x3 = COPY %9<br>
><br>
><br>
> ______________________________<wbr>_________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>