[PATCH] D35821: Have ARM call setBooleanContents(ZeroOrOneBooleanContent).

[Joel Galenson via Phabricator via llvm-commits]

jgalenson added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3674
+           dyn_cast<ConstantSDNode>(Tmp2.getOperand(1))->getZExtValue() == 1))
+        Tmp3 = Tmp2;
+      else
----------------
efriedma wrote:
> An AND of an AND should get optimized away by DAGCombine, I think?  Why isn't that happening?
It is getting optimized away by DAGCombine.  But in the wide-compares.ll test (in case I need to run it again) that happens right after it tries to combine a brcond, which fails to recognize the pattern if there are two ands (which feed into the brcond).  So it happens too late and the code for that test becomes slightly worse.


================
Comment at: test/CodeGen/ARM/cse-call.ll:5
 
-; Don't CSE a cmp across a call that clobbers CPSR.
-;
-; CHECK: cmp
 ; CHECK: S_trimzeros
 ; CHECK: cmp
----------------
efriedma wrote:
> If you delete the check line, this is no longer testing what it's supposed to test (which is that we don't CSE the cmp across the call).  You might need to use an icmp to get the required output here.
Good idea.  That worked.


================
Comment at: test/CodeGen/ARM/i1.ll:11
+entry:
+  br i1 undef, label %t, label %f
+
----------------
efriedma wrote:
> What are you trying to test here?
I couldn't easily come up with a simple test for this, so if you have any suggestions I'd be happy to try them instead.  But my goal was to test that the compiler ensures that the i1 is either 0 or 1 (since this commit says it must be one of them).  So my check for that first "mov" is the important part that is not there without my patch.


================
Comment at: test/CodeGen/Thumb2/2009-09-28-ITBlockBug.ll:10
+; CHECK:      it ne
+; CHECK-NEXT: bxne
 entry:
----------------
efriedma wrote:
> This test is ridiculous; we should just delete the whole file.
Yeah, that's a pretty crazy file.  Sounds good to me.


================
Comment at: test/CodeGen/Thumb2/2010-11-22-EpilogueBug.ll:11
 ; CHECK: push
-; CHECK: mov r7, sp
+; CHECK: add r7, sp
 ; CHECK: sub sp, #4
----------------
efriedma wrote:
> This looks weird; what is the assembly for the whole function?
_foo:
        push    {r4, r7, lr}
        add     r7, sp, #4
        sub     sp, #4
        movs    r4, #0
LBB0_1:                                 @ %bb
        cbnz    r4, LBB0_3
        bl      _bar
        cmp     r4, #0
        bne     LBB0_1
LBB0_3:                                 @ %return
        add     sp, #4
        pop     {r4, r7, pc}

Without this patch, that "movs r4, #0" isn't needed, so r4 isn't used, so the "add r7, sp, #4" is "mov r7, sp":

_foo:
        push    {r7, lr}
        mov     r7, sp
        sub     sp, #4
LBB0_1:                                 @ %bb
        cbnz    r0, LBB0_3
        bl      _bar
        cmp     r0, #0
        bne     LBB0_1
LBB0_3:                                 @ %return
        add     sp, #4
        pop     {r7, pc}


https://reviews.llvm.org/D35821