[TargetLowering][ARM] Don't alter opaque constants in TargetLowering::ShrinkDemandedConstant.

[craig.topper]

Summary

We don't constant fold based on demanded bits elsewhere in
SimplifyDemandedBits, so I don't think we should shrink them either.

The affected ARM test changes because a constant become non-opaque
and eventually enabled some constant folding. This no longer happens.
I checked and InstCombine is able to simplify this test. I'm not sure exactly
what it was trying to test.

[dmgreen]

Sounds OK to me, for my understanding of opaque constants

[rogfer01]

The affected ARM test changes because a constant become non-opaque
and eventually enabled some constant folding. This no longer happens.
I checked and InstCombine is able to simplify this test. I'm not sure exactly
what it was trying to test.

Looking at the original diff, back then we only cared about small subset of it where we checked that some constants were materialised in efficient ways.

 define i1 @t11() {
 entry:
   %bit = alloca i32
   %load = load i32, i32* %bit
   %clear = and i32 %load, -4096
   %set = or i32 %clear, 33
   store i32 %set, i32* %bit
   %load1 = load i32, i32* %bit
   %clear2 = and i32 %load1, -33550337
   %set3 = or i32 %clear2, 40960
   %clear5 = and i32 %set3, 4095
   %rem = srem i32 %clear5, 10
   %clear9 = and i32 %set3, -4096
   %set10 = or i32 %clear9, %rem
   store i32 %set10, i32* %bit
   %clear12 = and i32 %set10, 4095
   %cmp = icmp eq i32 %clear12, 3
   ret i1 %cmp
 
 ; ARM-LABEL: t11:
-; ARM: mov     r0, #0
-; ARM: cmp     r1, #3
-; ARM: moveq   r0, #1
+; ARM: rsbs r1, r0, #0
+; ARM: adc  r0, r0, r1
 
 ; ARMT2-LABEL: t11:
-; ARMT2: mov     r0, #0
-; ARMT2: cmp     r1, #3
-; ARMT2: movweq  r0, #1
+; ARMT2: clz r0, r0
+; ARMT2: lsr r0, r0, #5
 
 ; THUMB1-LABEL: t11:
 ; THUMB1-NOT: movs r0, #0
 ; THUMB1: movs r0, #5
 
 ; THUMB2-LABEL: t11:
-; THUMB2: movs    r0, #0
-; THUMB2: cmp     r1, #3
-; THUMB2: it      eq
-; THUMB2: moveq   r0, #1
+; THUMB2: clz r0, r0
+; THUMB2: lsrs r0, r0, #5
 }

The thumb checks (ARMT2 and THUMB*) seem still to be there. (The Arm expectation somehow seems to have been gone since 9d2df964070700ae0d244e84572ac2275050e49a in a change that seems positive overall)

When update_llc_test_check.py was applied it also included some part of the test that I understand it wasn't originally relevant.

The culprit seems to be a bitcast introduced by consthoist but I haven't tried to understand what it is doing to be honest. It looks like it is trying hard to push a constant into a (named) value (perhaps we want the constant materialised at all costs) and bitcast is a hackish way to achieve this?

*** IR Dump Before Constant Hoisting (consthoist) ***
define i1 @t11() #0 {
entry:
  %bit = alloca i32, align 4
  %load = load i32, i32* %bit, align 4
  %clear = and i32 %load, -4096
  %set = or i32 %clear, 33
  store i32 %set, i32* %bit, align 4
  %load1 = load i32, i32* %bit, align 4
  %clear2 = and i32 %load1, -33550337
  %set3 = or i32 %clear2, 40960
  %clear5 = and i32 %set3, 4095
  %rem = srem i32 %clear5, 10
  %clear9 = and i32 %set3, -4096
  %set10 = or i32 %clear9, %rem
  store i32 %set10, i32* %bit, align 4
  %clear12 = and i32 %set10, 4095
  %cmp = icmp eq i32 %clear12, 3
  ret i1 %cmp
}
*** IR Dump After Constant Hoisting (consthoist) ***
define i1 @t11() #0 {
entry:
  %const1 = bitcast i32 -4096 to i32  ; ---
  %const = bitcast i32 4095 to i32    ; ---
  %bit = alloca i32, align 4
  %load = load i32, i32* %bit, align 4
  %clear = and i32 %load, %const1
  %set = or i32 %clear, 33
  store i32 %set, i32* %bit, align 4
  %load1 = load i32, i32* %bit, align 4
  %clear2 = and i32 %load1, -33550337
  %set3 = or i32 %clear2, 40960
  %clear5 = and i32 %set3, %const
  %rem = srem i32 %clear5, 10
  %clear9 = and i32 %set3, %const1
  %set10 = or i32 %clear9, %rem
  store i32 %set10, i32* %bit, align 4
  %clear12 = and i32 %set10, %const
  %cmp = icmp eq i32 %clear12, 3
  ret i1 %cmp
}

[lebedev.ri]

In D104832#2837990, @dmgreen wrote:

Sounds OK to me, for my understanding of opaque constants

Same

[dmgreen]

In D104832#2838001, @rogfer01 wrote:

The culprit seems to be a bitcast introduced by consthoist but I haven't tried to understand what it is doing to be honest. It looks like it is trying hard to push a constant into a (named) value (perhaps we want the constant materialised at all costs) and bitcast is a hackish way to achieve this?

Yep. Constant hoisting pulls expensive constants out of the code, in order to only materialize them once. It uses a bitcast to do that, which in turn is expected to produce an opaque constant. That never seemed like a great way of doing it to me, but apparently it is fairly important for codesize in some cases. It's controlled through the costs of getIntImmCostInst, and v6m will have fairly high costs for a lot of constants.

I don't think there is much point in pulling constants out into the same block, but that seems like a separate issue.

[rogfer01]

In D104832#2838062, @dmgreen wrote:

In D104832#2838001, @rogfer01 wrote:

The culprit seems to be a bitcast introduced by consthoist but I haven't tried to understand what it is doing to be honest. It looks like it is trying hard to push a constant into a (named) value (perhaps we want the constant materialised at all costs) and bitcast is a hackish way to achieve this?

Yep. Constant hoisting pulls expensive constants out of the code, in order to only materialize them once. It uses a bitcast to do that, which in turn is expected to produce an opaque constant. That never seemed like a great way of doing it to me, but apparently it is fairly important for codesize in some cases. It's controlled through the costs of getIntImmCostInst, and v6m will have fairly high costs for a lot of constants.

I don't think there is much point in pulling constants out into the same block, but that seems like a separate issue.

Thanks a lot for the explanation David, makes sense.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

bool TargetLowering::ShrinkDemandedConstant(SDValue Op,
   // FIXME: ISD::SELECT, ISD::SELECT_CC
   switch (Opcode) {
   default:
     break;
   case ISD::XOR:
   case ISD::AND:
   case ISD::OR: {
     auto *Op1C = dyn_cast<ConstantSDNode>(Op.getOperand(1));
-    if (!Op1C)
+    if (!Op1C || Op1C->isOpaque())
       return false;
 
     // If this is a 'not' op, don't touch it because that's a canonical form.
     const APInt &C = Op1C->getAPIntValue();
     if (Opcode == ISD::XOR && DemandedBits.isSubsetOf(C))
       return false;
 
     if (!C.isSubsetOf(DemandedBits)) {

llvm/test/CodeGen/ARM/select-imm.ll

 ; ARMT2-NEXT:    sub r0, r0, #3
 ; ARMT2-NEXT:    clz r0, r0
 ; ARMT2-NEXT:    lsr r0, r0, #5
 ; ARMT2-NEXT:    add sp, sp, #4
 ; ARMT2-NEXT:    bx lr
 ;
 ; THUMB1-LABEL: t11:
 ; THUMB1:       @ %bb.0: @ %entry
-; THUMB1-NEXT:    .pad #4
-; THUMB1-NEXT:    sub sp, #4
+; THUMB1-NEXT:    .save {r4, r5, r7, lr}
+; THUMB1-NEXT:    push {r4, r5, r7, lr}
+; THUMB1-NEXT:    .pad #8
+; THUMB1-NEXT:    sub sp, #8
+; THUMB1-NEXT:    movs r4, #33
+; THUMB1-NEXT:    ldr r0, [sp, #4]
+; THUMB1-NEXT:    orrs r0, r4
+; THUMB1-NEXT:    ldr r1, .LCPI10_0
+; THUMB1-NEXT:    ands r1, r0
 ; THUMB1-NEXT:    movs r0, #5
 ; THUMB1-NEXT:    lsls r0, r0, #13
-; THUMB1-NEXT:    ldr r1, [sp]
-; THUMB1-NEXT:    orrs r1, r0
-; THUMB1-NEXT:    ldr r0, .LCPI10_0
-; THUMB1-NEXT:    ands r0, r1
-; THUMB1-NEXT:    adds r0, r0, #3
-; THUMB1-NEXT:    str r0, [sp]
-; THUMB1-NEXT:    movs r1, #0
+; THUMB1-NEXT:    adds r5, r1, r0
+; THUMB1-NEXT:    movs r1, #10
+; THUMB1-NEXT:    mov r0, r4
+; THUMB1-NEXT:    bl __aeabi_uidivmod
+; THUMB1-NEXT:    bics r5, r4
+; THUMB1-NEXT:    orrs r5, r1
+; THUMB1-NEXT:    str r5, [sp, #4]
+; THUMB1-NEXT:    ldr r0, .LCPI10_1
+; THUMB1-NEXT:    ands r0, r5
+; THUMB1-NEXT:    subs r1, r0, #3
 ; THUMB1-NEXT:    rsbs r0, r1, #0
 ; THUMB1-NEXT:    adcs r0, r1
-; THUMB1-NEXT:    add sp, #4
-; THUMB1-NEXT:    bx lr
+; THUMB1-NEXT:    add sp, #8
+; THUMB1-NEXT:    pop {r4, r5, r7, pc}
 ; THUMB1-NEXT:    .p2align 2
 ; THUMB1-NEXT:  @ %bb.1:
 ; THUMB1-NEXT:  .LCPI10_0:
-; THUMB1-NEXT:    .long 4261453824 @ 0xfe00a000
+; THUMB1-NEXT:    .long 4261412897 @ 0xfe000021
+; THUMB1-NEXT:  .LCPI10_1:
+; THUMB1-NEXT:    .long 4095 @ 0xfff
 ;
 ; THUMB2-LABEL: t11:
 ; THUMB2:       @ %bb.0: @ %entry
 ; THUMB2-NEXT:    .pad #4
 ; THUMB2-NEXT:    sub sp, #4
 ; THUMB2-NEXT:    ldr r1, [sp]
 ; THUMB2-NEXT:    movs r0, #33
 ; THUMB2-NEXT:    movw r2, #52429

 ; THUMB2-NEXT:    lsrs r0, r0, #5
 ; THUMB2-NEXT:    add sp, #4
 ; THUMB2-NEXT:    bx lr
 ;
 ; V8MBASE-LABEL: t11:
 ; V8MBASE:       @ %bb.0: @ %entry
 ; V8MBASE-NEXT:    .pad #4
 ; V8MBASE-NEXT:    sub sp, #4
-; V8MBASE-NEXT:    movw r0, #40960
+; V8MBASE-NEXT:    movs r0, #127
+; V8MBASE-NEXT:    lsls r0, r0, #25
 ; V8MBASE-NEXT:    ldr r1, [sp]
-; V8MBASE-NEXT:    orrs r1, r0
-; V8MBASE-NEXT:    movw r0, #40960
-; V8MBASE-NEXT:    movt r0, #65024
-; V8MBASE-NEXT:    ands r0, r1
-; V8MBASE-NEXT:    adds r0, r0, #3
+; V8MBASE-NEXT:    ands r1, r0
+; V8MBASE-NEXT:    movw r0, #40963
+; V8MBASE-NEXT:    adds r0, r1, r0
 ; V8MBASE-NEXT:    str r0, [sp]
 ; V8MBASE-NEXT:    movs r1, #0
 ; V8MBASE-NEXT:    rsbs r0, r1, #0
 ; V8MBASE-NEXT:    adcs r0, r1
 ; V8MBASE-NEXT:    add sp, #4
 ; V8MBASE-NEXT:    bx lr
 entry:
   %bit = alloca i32