[x86] Relax the check in areLoadsFromSameBasePtr

[eraman]

Summary

The code returns false if the Scale value (which should match in Load1 and Load2) is not 1. I think all that is required is every operand except the displacement must be identical.

[mkuper]

This makes sense to me, but I don't understand why the Scale == 1 requirement was there to begin with.
+ctopper for another look.

[craig.topper]

Evan Cheng added this in r94147 at that time there was an additional check on the index register itself too. This is what the "Index should be Reg0" comment was referring too, but I don't know what that means. Its before my time. Jakob Stoklund Olesen removed that check in r100497, but did not update the comment about Reg0.

I think this change is fine.

While you're touching this can you use the X86::AddrBaseReg, X86::AddrScaleAmt, etc. constants when calling getOperand. This will improve readability here.

[eraman]

Used getOperand(X86::AddrBaseReg) etc instead of getOperand(0). Also removed if the chain operands match. This check shouldn't be in areLoadsFromSameBasePtr and the caller already passes two loads that are uses of the same chain node.

[craig.topper]

LGTM

[chapuni]

Seems it causes crash in stage2.
http://bb.pgr.jp/builders/clang-3stage-x86_64-linux/builds/14942

As far as I investigated, this change misses the case "Load1->getOperand(5) != Load2->getOperand(5)"

t69: i64,ch = <<Unknown Machine Node #63812>><Mem:LD8[%_M_p.i.i.i.i.i34](tbaa=<0xa62bec8>)> t2, TargetConstant:i8<1>, Register:i64 %noreg, TargetConstant:i32<0>, Register:i32 %noreg, t2:1
t78: i64,ch = <<Unknown Machine Node #63812>><Mem:LD8[%9](tbaa=<0xa55faf8>)> t2, TargetConstant:i8<1>, Register:i64 %noreg, TargetConstant:i32<56>, Register:i32 %noreg, t29:1

Is it true for areLoadsFromSameBasePtr() ?

llvm/trunk/lib/Target/X86/X86InstrInfo.cpp

8985 ↗

(On Diff #94892)

Crashes can be fixed if this logic is included.

lib/Target/X86/X86InstrInfo.cpp

X86InstrInfo::areLoadsFromSameBasePtr(SDNode *Load1, SDNode *Load2,
   case X86::VMOVDQU64Zrm:
   case X86::KMOVBkm:
   case X86::KMOVWkm:
   case X86::KMOVDkm:
   case X86::KMOVQkm:
     break;
   }
 
-  // Check if chain operands and base addresses match.
-  if (Load1->getOperand(0) != Load2->getOperand(0) ||
-      Load1->getOperand(5) != Load2->getOperand(5))
-    return false;
-  // Segment operands should match as well.
-  if (Load1->getOperand(4) != Load2->getOperand(4))
-    return false;
-  // Scale should be 1, Index should be Reg0.
-  if (Load1->getOperand(1) == Load2->getOperand(1) &&
-      Load1->getOperand(2) == Load2->getOperand(2)) {
-    if (cast<ConstantSDNode>(Load1->getOperand(1))->getZExtValue() != 1)
-      return false;
-
-    // Now let's examine the displacements.
-    if (isa<ConstantSDNode>(Load1->getOperand(3)) &&
-        isa<ConstantSDNode>(Load2->getOperand(3))) {
-      Offset1 = cast<ConstantSDNode>(Load1->getOperand(3))->getSExtValue();
-      Offset2 = cast<ConstantSDNode>(Load2->getOperand(3))->getSExtValue();
-      return true;
-    }
-  }
+  // Lambda to check if both the loads have the same value for an operand index.
+  auto HasSameOp = [&](int I) {
+    return Load1->getOperand(I) == Load2->getOperand(I);
+  };
+
+  // All operands except the displacement should match.
+  if (!HasSameOp(X86::AddrBaseReg) || !HasSameOp(X86::AddrScaleAmt) ||
+      !HasSameOp(X86::AddrIndexReg) || !HasSameOp(X86::AddrSegmentReg))
     return false;
+
+  // Now let's examine if the displacements are constants.
+  auto Disp1 = dyn_cast<ConstantSDNode>(Load1->getOperand(X86::AddrDisp));
+  auto Disp2 = dyn_cast<ConstantSDNode>(Load2->getOperand(X86::AddrDisp));
+  if (!Disp1 || !Disp2)
+    return false;
+
+  Offset1 = Disp1->getSExtValue();
+  Offset2 = Disp2->getSExtValue();
+  return true;
 }
 
 bool X86InstrInfo::shouldScheduleLoadsNear(SDNode *Load1, SDNode *Load2,
                                            int64_t Offset1, int64_t Offset2,
                                            unsigned NumLoads) const {
   assert(Offset2 > Offset1);
   if ((Offset2 - Offset1) / 8 > 64)
     return false;

test/CodeGen/X86/load-slice.ll

 ;       Low      High
 ; The base address points to 0 and is 8-bytes aligned.
 ; Low slice starts at 0 (base) and is 8-bytes aligned.
 ; High slice starts at 4 (base + 4-bytes) and is 4-bytes aligned.
 ;
 ; STRESS-LABEL: t1:
 ; Load out[out_start + 8].real, this is base + 8 * 8 + 0.
 ; STRESS: vmovss 64([[BASE:[^(]+]]), [[OUT_Real:%xmm[0-9]+]]
-; Add low slice: out[out_start].real, this is base + 0.
-; STRESS-NEXT: vaddss ([[BASE]]), [[OUT_Real]], [[RES_Real:%xmm[0-9]+]]
 ; Load out[out_start + 8].imm, this is base + 8 * 8 + 4.
 ; STRESS-NEXT: vmovss 68([[BASE]]), [[OUT_Imm:%xmm[0-9]+]]
+; Add low slice: out[out_start].real, this is base + 0.
+; STRESS-NEXT: vaddss ([[BASE]]), [[OUT_Real]], [[RES_Real:%xmm[0-9]+]]
 ; Add high slice: out[out_start].imm, this is base + 4.
 ; STRESS-NEXT: vaddss 4([[BASE]]), [[OUT_Imm]], [[RES_Imm:%xmm[0-9]+]]
 ; Swap Imm and Real.
 ; STRESS-NEXT: vinsertps $16, [[RES_Imm]], [[RES_Real]], [[RES_Vec:%xmm[0-9]+]]
 ; Put the results back into out[out_start].
 ; STRESS-NEXT: vmovlps [[RES_Vec]], ([[BASE]])
 ;
 ; Same for REGULAR, we eliminate register bank copy with each slices.
 ; REGULAR-LABEL: t1:
 ; Load out[out_start + 8].real, this is base + 8 * 8 + 0.
 ; REGULAR: vmovss 64([[BASE:[^)]+]]), [[OUT_Real:%xmm[0-9]+]]
-; Add low slice: out[out_start].real, this is base + 0.
-; REGULAR-NEXT: vaddss ([[BASE]]), [[OUT_Real]], [[RES_Real:%xmm[0-9]+]]
 ; Load out[out_start + 8].imm, this is base + 8 * 8 + 4.
 ; REGULAR-NEXT: vmovss 68([[BASE]]), [[OUT_Imm:%xmm[0-9]+]]
+; Add low slice: out[out_start].real, this is base + 0.
+; REGULAR-NEXT: vaddss ([[BASE]]), [[OUT_Real]], [[RES_Real:%xmm[0-9]+]]
 ; Add high slice: out[out_start].imm, this is base + 4.
 ; REGULAR-NEXT: vaddss 4([[BASE]]), [[OUT_Imm]], [[RES_Imm:%xmm[0-9]+]]
 ; Swap Imm and Real.
 ; REGULAR-NEXT: vinsertps $16, [[RES_Imm]], [[RES_Real]], [[RES_Vec:%xmm[0-9]+]]
 ; Put the results back into out[out_start].
 ; REGULAR-NEXT: vmovlps [[RES_Vec]], ([[BASE]])
 define void @t1(%class.Complex* nocapture %out, i64 %out_start) {
 entry: