[DAGCombine] Fix for PR35763

[samparker]

Summary

While searching for loads to be narrowed, equal sized loads were not added to the list, resulting in anyext loads not being converted to zext loads.

https://bugs.llvm.org/show_bug.cgi?id=35763

[RKSimon]

test/CodeGen/X86/pr35763.ll

2

Please use utils\update_llc_test_checks.py to regenerate the checks

[samparker]

Thanks Simon, I've updated the test.

[RKSimon]

Does pr35763.ll reduce any further?

[samparker]

Not simply, the original test case is a bit odd and the bug easily fails to trigger. Any particular reason for not using the original reduced test case?

[RKSimon]

In D41628#965775, @samparker wrote:

Not simply, the original test case is a bit odd and the bug easily fails to trigger. Any particular reason for not using the original reduced test case?

Just that more complex test cases can sometimes result in the underlying issue it was testing being 'hidden' by future (unrelated) patches, the simpler the test case the better likelihood that we're still testing for the same thing.

Please can you commit pr35763.ll with the trunk's current codegen and then update this patch just to show the codegen diff?

[samparker]

Ok. I haven't got commit access right now, but I'll do it first thing tomorrow morning.

[samparker]

Committed pr37563.ll so this patch can show the changes.

[RKSimon]

LGTM - thanks

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

       return false;
 
     switch(Op.getOpcode()) {
     case ISD::LOAD: {
       auto *Load = cast<LoadSDNode>(Op);
       EVT ExtVT;
       if (isAndLoadExtLoad(Mask, Load, Load->getValueType(0), ExtVT) &&
           isLegalNarrowLoad(Load, ISD::ZEXTLOAD, ExtVT)) {
-        // Only add this load if we can make it more narrow.
-        if (ExtVT.bitsLT(Load->getMemoryVT()))
+
+        // ZEXTLOAD is already small enough.
+        if (Load->getExtensionType() == ISD::ZEXTLOAD &&
+            ExtVT.bitsGE(Load->getMemoryVT()))
+          continue;
+
+        // Use LE to convert equal sized loads to zext.
+        if (ExtVT.bitsLE(Load->getMemoryVT()))
           Loads.insert(Load);
+
         continue;
       }
       return false;
     }
     case ISD::ZERO_EXTEND:
     case ISD::AssertZext: {
       unsigned ActiveBits = Mask->getAPIntValue().countTrailingOnes();
       EVT ExtVT = EVT::getIntegerVT(*DAG.getContext(), ActiveBits);

 
   SmallPtrSet<LoadSDNode*, 8> Loads;
   SmallPtrSet<SDNode*, 2> NodesWithConsts;
   SDNode *FixupNode = nullptr;
   if (SearchForAndLoads(N, Loads, NodesWithConsts, Mask, FixupNode)) {
     if (Loads.size() == 0)
       return false;
 
+    DEBUG(dbgs() << "Backwards propagate AND: "; N->dump());
     SDValue MaskOp = N->getOperand(1);
 
     // If it exists, fixup the single node we allow in the tree that needs
     // masking.
     if (FixupNode) {
+      DEBUG(dbgs() << "First, need to fix up: "; FixupNode->dump());
       SDValue And = DAG.getNode(ISD::AND, SDLoc(FixupNode),
                                 FixupNode->getValueType(0),
                                 SDValue(FixupNode, 0), MaskOp);
       DAG.ReplaceAllUsesOfValueWith(SDValue(FixupNode, 0), And);
       DAG.UpdateNodeOperands(And.getNode(), SDValue(FixupNode, 0),
                              MaskOp);
     }
 
     // Narrow any constants that need it.
     for (auto *LogicN : NodesWithConsts) {
       auto *C = cast<ConstantSDNode>(LogicN->getOperand(1));
       SDValue And = DAG.getNode(ISD::AND, SDLoc(C), C->getValueType(0),
                                 SDValue(C, 0), MaskOp);
       DAG.UpdateNodeOperands(LogicN, LogicN->getOperand(0), And);
     }
 
     // Create narrow loads.
     for (auto *Load : Loads) {
+      DEBUG(dbgs() << "Propagate AND back to: "; Load->dump());
       SDValue And = DAG.getNode(ISD::AND, SDLoc(Load), Load->getValueType(0),
                                 SDValue(Load, 0), MaskOp);
       DAG.ReplaceAllUsesOfValueWith(SDValue(Load, 0), And);
       DAG.UpdateNodeOperands(And.getNode(), SDValue(Load, 0), MaskOp);
       SDValue NewLoad = ReduceLoadWidth(And.getNode());
       assert(NewLoad &&
              "Shouldn't be masking the load if it can't be narrowed");
       CombineTo(Load, NewLoad, NewLoad.getValue(1));

test/CodeGen/ARM/and-load-combine.ll

   ret i32 %and
 }
 
 define arm_aapcscc i1 @test6(i8* %x, i8 %y, i8 %z) {
 ; ARM-LABEL: test6:
 ; ARM:       @ %bb.0: @ %entry
 ; ARM-NEXT:    ldrb r0, [r0]
 ; ARM-NEXT:    uxtb r2, r2
-; ARM-NEXT:    and r0, r0, r1
-; ARM-NEXT:    uxtb r1, r0
+; ARM-NEXT:    and r1, r0, r1
 ; ARM-NEXT:    mov r0, #0
 ; ARM-NEXT:    cmp r1, r2
 ; ARM-NEXT:    movweq r0, #1
 ; ARM-NEXT:    bx lr
 ;
 ; ARMEB-LABEL: test6:
 ; ARMEB:       @ %bb.0: @ %entry
 ; ARMEB-NEXT:    ldrb r0, [r0]
 ; ARMEB-NEXT:    uxtb r2, r2
-; ARMEB-NEXT:    and r0, r0, r1
-; ARMEB-NEXT:    uxtb r1, r0
+; ARMEB-NEXT:    and r1, r0, r1
 ; ARMEB-NEXT:    mov r0, #0
 ; ARMEB-NEXT:    cmp r1, r2
 ; ARMEB-NEXT:    movweq r0, #1
 ; ARMEB-NEXT:    bx lr
 ;
 ; THUMB1-LABEL: test6:
 ; THUMB1:       @ %bb.0: @ %entry
-; THUMB1-NEXT:    ldrb r0, [r0]
-; THUMB1-NEXT:    ands r0, r1
-; THUMB1-NEXT:    uxtb r3, r0
+; THUMB1-NEXT:    ldrb r3, [r0]
+; THUMB1-NEXT:    ands r3, r1
 ; THUMB1-NEXT:    uxtb r2, r2
 ; THUMB1-NEXT:    movs r0, #1
 ; THUMB1-NEXT:    movs r1, #0
 ; THUMB1-NEXT:    cmp r3, r2
 ; THUMB1-NEXT:    beq .LBB18_2
 ; THUMB1-NEXT:  @ %bb.1: @ %entry
 ; THUMB1-NEXT:    mov r0, r1
 ; THUMB1-NEXT:  .LBB18_2: @ %entry
 ; THUMB1-NEXT:    bx lr
 ;
 ; THUMB2-LABEL: test6:
 ; THUMB2:       @ %bb.0: @ %entry
 ; THUMB2-NEXT:    ldrb r0, [r0]
 ; THUMB2-NEXT:    uxtb r2, r2
-; THUMB2-NEXT:    ands r0, r1
-; THUMB2-NEXT:    uxtb r1, r0
+; THUMB2-NEXT:    ands r1, r0
 ; THUMB2-NEXT:    movs r0, #0
 ; THUMB2-NEXT:    cmp r1, r2
 ; THUMB2-NEXT:    it eq
 ; THUMB2-NEXT:    moveq r0, #1
 ; THUMB2-NEXT:    bx lr
 entry:
   %0 = load i8, i8* %x, align 4
   %1 = and i8 %0, %y

test/CodeGen/X86/pr35763.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=x86_64-linux-gnu %s -o - | FileCheck %s
+
+%struct.S = type <{ i16, i24, [5 x i8], i8, i16, [2 x i8] }>
+
+@z = global { i16, i8, i8, i8, i8, i8, i8, i8, i8, i8, [5 x i8] } { i16 -724, i8 94, i8 -18, i8 5, i8 undef, i8 96, i8 104, i8 -24, i8 10, i8 0, [5 x i8] undef }, align 8
+@tf_3_var_136 = global i64 0, align 8
+@.str = private unnamed_addr constant [6 x i8] c"%llu\0A\00", align 1
+
+define void @PR35763() {
+; CHECK-LABEL: PR35763:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    movzwl {{.*}}(%rip), %eax
+; CHECK-NEXT:    movzwl z+{{.*}}(%rip), %ecx
+; CHECK-NEXT:    orl %eax, %ecx
+; CHECK-NEXT:    movq %rcx, {{.*}}(%rip)
+; CHECK-NEXT:    movl z+{{.*}}(%rip), %eax
+; CHECK-NEXT:    movzbl z+{{.*}}(%rip), %ecx
+; CHECK-NEXT:    shlq $32, %rcx
+; CHECK-NEXT:    orq %rax, %rcx
+; CHECK-NEXT:    movabsq $1090921758719, %rax # imm = 0xFE0000FFFF
+; CHECK-NEXT:    andq %rcx, %rax
+; CHECK-NEXT:    movl %eax, z+{{.*}}(%rip)
+; CHECK-NEXT:    shrq $32, %rax
+; CHECK-NEXT:    movb %al, z+{{.*}}(%rip)
+; CHECK-NEXT:    retq
+entry:
+  %0 = load i16, i16* getelementptr inbounds (%struct.S, %struct.S* bitcast ({ i16, i8, i8, i8, i8, i8, i8, i8, i8, i8, [5 x i8] }* @z to %struct.S*), i32 0, i32 0), align 8
+  %conv = sext i16 %0 to i32
+  %bf.load = load i32, i32* bitcast (i24* getelementptr inbounds (%struct.S, %struct.S* bitcast ({ i16, i8, i8, i8, i8, i8, i8, i8, i8, i8, [5 x i8] }* @z to %struct.S*), i32 0, i32 1) to i32*), align 2
+  %bf.clear = and i32 %bf.load, 2097151
+  %bf.cast = zext i32 %bf.clear to i64
+  %conv1 = trunc i64 %bf.cast to i32
+  %or = or i32 %conv, %conv1
+  %conv2 = trunc i32 %or to i16
+  %conv3 = zext i16 %conv2 to i64
+  store i64 %conv3, i64* @tf_3_var_136, align 8
+  %bf.load4 = load i40, i40* bitcast ([5 x i8]* getelementptr inbounds (%struct.S, %struct.S* bitcast ({ i16, i8, i8, i8, i8, i8, i8, i8, i8, i8, [5 x i8] }* @z to %struct.S*), i32 0, i32 2) to i40*), align 2
+  %bf.clear5 = and i40 %bf.load4, -8589869057
+  store i40 %bf.clear5, i40* bitcast ([5 x i8]* getelementptr inbounds (%struct.S, %struct.S* bitcast ({ i16, i8, i8, i8, i8, i8, i8, i8, i8, i8, [5 x i8] }* @z to %struct.S*), i32 0, i32 2) to i40*), align 2
+  ret void
+}