Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AArch64] Reland merge index address with large offset into base address #79951

Merged
merged 2 commits into from
Aug 15, 2024

Conversation

vfdff
Copy link
Contributor

@vfdff vfdff commented Jan 30, 2024

A case for this transformation, https://gcc.godbolt.org/z/nhYcWq1WE

Fold
  mov     w8, https://github.com/llvm/llvm-project/issues/56952
  movk    w8, https://github.com/llvm/llvm-project/pull/15, lsl https://github.com/llvm/llvm-project/pull/16
  ldrb    w0, [x0, x8]
into
  add     x0, x0, 1036288
  ldrb    w0, [x0, 3704]

Only LDRBBroX is supported for the first time.
Fix #71917
Fix #76202

@llvmbot
Copy link
Member

llvmbot commented Jan 30, 2024

@llvm/pr-subscribers-backend-aarch64

Author: Allen (vfdff)

Changes

A case for this transformation, https://gcc.godbolt.org/z/nhYcWq1WE

Fold
  mov     w8, https://github.com/llvm/llvm-project/issues/56952
  movk    w8, https://github.com/llvm/llvm-project/pull/15, lsl https://github.com/llvm/llvm-project/pull/16
  ldrb    w0, [x0, x8]
into
  add     x0, x0, 1036288
  ldrb    w0, [x0, 3704]

Only LDRBBroX is supported for the first time.
Fix #71917


Full diff: https://github.com/llvm/llvm-project/pull/79951.diff

5 Files Affected:

  • (modified) llvm/lib/Target/AArch64/AArch64InstrInfo.cpp (+10)
  • (modified) llvm/lib/Target/AArch64/AArch64InstrInfo.h (+3)
  • (modified) llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp (+232)
  • (modified) llvm/test/CodeGen/AArch64/arm64-addrmode.ll (+6-9)
  • (modified) llvm/test/CodeGen/AArch64/large-offset-ldr-merge.mir (+22-3)
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index 13e9d9725cc2e..2e8d8c63d6bec 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -4098,6 +4098,16 @@ AArch64InstrInfo::getLdStOffsetOp(const MachineInstr &MI) {
   return MI.getOperand(Idx);
 }
 
+const MachineOperand &
+AArch64InstrInfo::getLdStAmountOp(const MachineInstr &MI) {
+  switch (MI.getOpcode()) {
+  default:
+    llvm_unreachable("Unexpected opcode");
+  case AArch64::LDRBBroX:
+    return MI.getOperand(4);
+  }
+}
+
 static const TargetRegisterClass *getRegClass(const MachineInstr &MI,
                                               Register Reg) {
   if (MI.getParent() == nullptr)
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.h b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
index 6526f6740747a..db24a19fe5f8e 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.h
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.h
@@ -111,6 +111,9 @@ class AArch64InstrInfo final : public AArch64GenInstrInfo {
   /// Returns the immediate offset operator of a load/store.
   static const MachineOperand &getLdStOffsetOp(const MachineInstr &MI);
 
+  /// Returns the shift amount operator of a load/store.
+  static const MachineOperand &getLdStAmountOp(const MachineInstr &MI);
+
   /// Returns whether the instruction is FP or NEON.
   static bool isFpOrNEON(const MachineInstr &MI);
 
diff --git a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
index 926a89466255c..8be19652edbb5 100644
--- a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
+++ b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
@@ -62,6 +62,8 @@ STATISTIC(NumUnscaledPairCreated,
           "Number of load/store from unscaled generated");
 STATISTIC(NumZeroStoresPromoted, "Number of narrow zero stores promoted");
 STATISTIC(NumLoadsFromStoresPromoted, "Number of loads from stores promoted");
+STATISTIC(NumConstOffsetFolded,
+          "Number of const offset of index address folded");
 
 DEBUG_COUNTER(RegRenamingCounter, DEBUG_TYPE "-reg-renaming",
               "Controls which pairs are considered for renaming");
@@ -75,6 +77,11 @@ static cl::opt<unsigned> LdStLimit("aarch64-load-store-scan-limit",
 static cl::opt<unsigned> UpdateLimit("aarch64-update-scan-limit", cl::init(100),
                                      cl::Hidden);
 
+// The LdStConstLimit limits how far we search for const offset instructions
+// when we form index address load/store instructions.
+static cl::opt<unsigned> LdStConstLimit("aarch64-load-store-const-scan-limit",
+                                        cl::init(10), cl::Hidden);
+
 // Enable register renaming to find additional store pairing opportunities.
 static cl::opt<bool> EnableRenaming("aarch64-load-store-renaming",
                                     cl::init(true), cl::Hidden);
@@ -171,6 +178,13 @@ struct AArch64LoadStoreOpt : public MachineFunctionPass {
   findMatchingUpdateInsnForward(MachineBasicBlock::iterator I,
                                 int UnscaledOffset, unsigned Limit);
 
+  // Scan the instruction list to find a register assigned with a const
+  // value that can be combined with the current instruction (a load or store)
+  // using base addressing with writeback. Scan forwards.
+  MachineBasicBlock::iterator
+  findMatchingConstOffsetBackward(MachineBasicBlock::iterator I, unsigned Limit,
+                                  unsigned &Offset);
+
   // Scan the instruction list to find a base register update that can
   // be combined with the current instruction (a load or store) using
   // pre or post indexed addressing with writeback. Scan backwards.
@@ -182,11 +196,19 @@ struct AArch64LoadStoreOpt : public MachineFunctionPass {
   bool isMatchingUpdateInsn(MachineInstr &MemMI, MachineInstr &MI,
                             unsigned BaseReg, int Offset);
 
+  bool isMatchingMovConstInsn(MachineInstr &MemMI, MachineInstr &MI,
+                              unsigned IndexReg, unsigned &Offset);
+
   // Merge a pre- or post-index base register update into a ld/st instruction.
   MachineBasicBlock::iterator
   mergeUpdateInsn(MachineBasicBlock::iterator I,
                   MachineBasicBlock::iterator Update, bool IsPreIdx);
 
+  MachineBasicBlock::iterator
+  mergeConstOffsetInsn(MachineBasicBlock::iterator I,
+                       MachineBasicBlock::iterator Update, unsigned Offset,
+                       int Scale);
+
   // Find and merge zero store instructions.
   bool tryToMergeZeroStInst(MachineBasicBlock::iterator &MBBI);
 
@@ -199,6 +221,9 @@ struct AArch64LoadStoreOpt : public MachineFunctionPass {
   // Find and merge a base register updates before or after a ld/st instruction.
   bool tryToMergeLdStUpdate(MachineBasicBlock::iterator &MBBI);
 
+  // Find and merge a index ldr/st instructions into a base ld/st instruction.
+  bool tryToMergeIndexLdSt(MachineBasicBlock::iterator &MBBI, int Scale);
+
   bool optimizeBlock(MachineBasicBlock &MBB, bool EnableNarrowZeroStOpt);
 
   bool runOnMachineFunction(MachineFunction &Fn) override;
@@ -481,6 +506,16 @@ static unsigned getPreIndexedOpcode(unsigned Opc) {
   }
 }
 
+static unsigned getBaseAddressOpcode(unsigned Opc) {
+  // TODO: Add more index address loads/stores.
+  switch (Opc) {
+  default:
+    llvm_unreachable("Opcode has no base address equivalent!");
+  case AArch64::LDRBBroX:
+    return AArch64::LDRBBui;
+  }
+}
+
 static unsigned getPostIndexedOpcode(unsigned Opc) {
   switch (Opc) {
   default:
@@ -722,6 +757,20 @@ static bool isMergeableLdStUpdate(MachineInstr &MI) {
   }
 }
 
+// Make sure this is a reg+reg Ld/St
+static bool isMergeableIndexLdSt(MachineInstr &MI, int &Scale) {
+  unsigned Opc = MI.getOpcode();
+  switch (Opc) {
+  default:
+    return false;
+  // Scaled instructions.
+  // TODO: Add more index address loads/stores.
+  case AArch64::LDRBBroX:
+    Scale = 1;
+    return true;
+  }
+}
+
 static bool isRewritableImplicitDef(unsigned Opc) {
   switch (Opc) {
   default:
@@ -2048,6 +2097,63 @@ AArch64LoadStoreOpt::mergeUpdateInsn(MachineBasicBlock::iterator I,
   return NextI;
 }
 
+MachineBasicBlock::iterator
+AArch64LoadStoreOpt::mergeConstOffsetInsn(MachineBasicBlock::iterator I,
+                                          MachineBasicBlock::iterator Update,
+                                          unsigned Offset, int Scale) {
+  assert((Update->getOpcode() == AArch64::MOVKWi) &&
+         "Unexpected const mov instruction to merge!");
+  MachineBasicBlock::iterator E = I->getParent()->end();
+  MachineBasicBlock::iterator NextI = next_nodbg(I, E);
+  MachineBasicBlock::iterator PrevI = prev_nodbg(Update, E);
+  MachineInstr &MemMI = *I;
+  unsigned Mask = (1 << 12) * Scale - 1;
+  unsigned Low = Offset & Mask;
+  unsigned High = Offset - Low;
+  Register BaseReg = AArch64InstrInfo::getLdStBaseOp(MemMI).getReg();
+  Register IndexReg = AArch64InstrInfo::getLdStOffsetOp(MemMI).getReg();
+  MachineInstrBuilder AddMIB, MemMIB;
+
+  // Add IndexReg, BaseReg, High (the BaseReg may be SP)
+  AddMIB =
+      BuildMI(*I->getParent(), I, I->getDebugLoc(), TII->get(AArch64::ADDXri))
+          .addDef(IndexReg)
+          .addUse(BaseReg)
+          .addImm(High >> 12) // shifted value
+          .addImm(12);        // shift 12
+  (void)AddMIB;
+  // Ld/St DestReg, IndexReg, Imm12
+  unsigned NewOpc = getBaseAddressOpcode(I->getOpcode());
+  MemMIB = BuildMI(*I->getParent(), I, I->getDebugLoc(), TII->get(NewOpc))
+               .add(getLdStRegOp(MemMI))
+               .add(AArch64InstrInfo::getLdStOffsetOp(MemMI))
+               .addImm(Low / Scale)
+               .setMemRefs(I->memoperands())
+               .setMIFlags(I->mergeFlagsWith(*Update));
+  (void)MemMIB;
+
+  ++NumConstOffsetFolded;
+  LLVM_DEBUG(dbgs() << "Creating base address load/store.\n");
+  LLVM_DEBUG(dbgs() << "    Replacing instructions:\n    ");
+  LLVM_DEBUG(PrevI->print(dbgs()));
+  LLVM_DEBUG(dbgs() << "    ");
+  LLVM_DEBUG(Update->print(dbgs()));
+  LLVM_DEBUG(dbgs() << "    ");
+  LLVM_DEBUG(I->print(dbgs()));
+  LLVM_DEBUG(dbgs() << "  with instruction:\n    ");
+  LLVM_DEBUG(((MachineInstr *)AddMIB)->print(dbgs()));
+  LLVM_DEBUG(dbgs() << "    ");
+  LLVM_DEBUG(((MachineInstr *)MemMIB)->print(dbgs()));
+  LLVM_DEBUG(dbgs() << "\n");
+
+  // Erase the old instructions for the block.
+  I->eraseFromParent();
+  PrevI->eraseFromParent();
+  Update->eraseFromParent();
+
+  return NextI;
+}
+
 bool AArch64LoadStoreOpt::isMatchingUpdateInsn(MachineInstr &MemMI,
                                                MachineInstr &MI,
                                                unsigned BaseReg, int Offset) {
@@ -2095,6 +2201,34 @@ bool AArch64LoadStoreOpt::isMatchingUpdateInsn(MachineInstr &MemMI,
   return false;
 }
 
+bool AArch64LoadStoreOpt::isMatchingMovConstInsn(MachineInstr &MemMI,
+                                                 MachineInstr &MI,
+                                                 unsigned IndexReg,
+                                                 unsigned &Offset) {
+  // The update instruction source and destination register must be the
+  // same as the load/store index register.
+  if (MI.getOpcode() == AArch64::MOVKWi &&
+      TRI->isSuperOrSubRegisterEq(IndexReg, MI.getOperand(1).getReg())) {
+
+    // movz + movk hold a large offset of a Ld/St instruction.
+    MachineBasicBlock::iterator B = MI.getParent()->begin();
+    MachineBasicBlock::iterator MBBI = &MI;
+    // Skip the scene when the MI is the first instruction of a block.
+    if (MBBI == B)
+      return false;
+    MBBI = prev_nodbg(MBBI, B);
+    MachineInstr &MovzMI = *MBBI;
+    if (MovzMI.getOpcode() == AArch64::MOVZWi) {
+      unsigned Low = MovzMI.getOperand(1).getImm();
+      unsigned High = MI.getOperand(2).getImm() << MI.getOperand(3).getImm();
+      Offset = High + Low;
+      // 12-bit optionally shifted immediates are legal for adds.
+      return Offset >> 24 == 0;
+    }
+  }
+  return false;
+}
+
 MachineBasicBlock::iterator AArch64LoadStoreOpt::findMatchingUpdateInsnForward(
     MachineBasicBlock::iterator I, int UnscaledOffset, unsigned Limit) {
   MachineBasicBlock::iterator E = I->getParent()->end();
@@ -2250,6 +2384,60 @@ MachineBasicBlock::iterator AArch64LoadStoreOpt::findMatchingUpdateInsnBackward(
   return E;
 }
 
+MachineBasicBlock::iterator
+AArch64LoadStoreOpt::findMatchingConstOffsetBackward(
+    MachineBasicBlock::iterator I, unsigned Limit, unsigned &Offset) {
+  MachineBasicBlock::iterator B = I->getParent()->begin();
+  MachineBasicBlock::iterator E = I->getParent()->end();
+  MachineInstr &MemMI = *I;
+  MachineBasicBlock::iterator MBBI = I;
+
+  // If the load is the first instruction in the block, there's obviously
+  // not any matching load or store.
+  if (MBBI == B)
+    return E;
+
+  // Make sure the IndexReg is killed and the shift amount is zero.
+  // TODO: Relex this restriction to extend, simplify processing now.
+  if (!AArch64InstrInfo::getLdStOffsetOp(MemMI).isKill() ||
+      !AArch64InstrInfo::getLdStAmountOp(MemMI).isImm() ||
+      (AArch64InstrInfo::getLdStAmountOp(MemMI).getImm() != 0))
+    return E;
+
+  Register IndexReg = AArch64InstrInfo::getLdStOffsetOp(MemMI).getReg();
+
+  // Track which register units have been modified and used between the first
+  // insn (inclusive) and the second insn.
+  ModifiedRegUnits.clear();
+  UsedRegUnits.clear();
+  unsigned Count = 0;
+  do {
+    MBBI = prev_nodbg(MBBI, B);
+    MachineInstr &MI = *MBBI;
+
+    // Don't count transient instructions towards the search limit since there
+    // may be different numbers of them if e.g. debug information is present.
+    if (!MI.isTransient())
+      ++Count;
+
+    // If we found a match, return it.
+    if (isMatchingMovConstInsn(*I, MI, IndexReg, Offset)) {
+      return MBBI;
+    }
+
+    // Update the status of what the instruction clobbered and used.
+    LiveRegUnits::accumulateUsedDefed(MI, ModifiedRegUnits, UsedRegUnits, TRI);
+
+    // Otherwise, if the index register is used or modified, we have no match,
+    // so return early.
+    if (!ModifiedRegUnits.available(IndexReg) ||
+        !UsedRegUnits.available(IndexReg))
+      return E;
+
+  } while (MBBI != B && Count < Limit);
+  return E;
+}
+
 bool AArch64LoadStoreOpt::tryToPromoteLoadFromStore(
     MachineBasicBlock::iterator &MBBI) {
   MachineInstr &MI = *MBBI;
@@ -2434,6 +2622,34 @@ bool AArch64LoadStoreOpt::tryToMergeLdStUpdate
   return false;
 }
 
+bool AArch64LoadStoreOpt::tryToMergeIndexLdSt(MachineBasicBlock::iterator &MBBI,
+                                              int Scale) {
+  MachineInstr &MI = *MBBI;
+  MachineBasicBlock::iterator E = MI.getParent()->end();
+  MachineBasicBlock::iterator Update;
+
+  // Don't know how to handle unscaled pre/post-index versions below, so bail.
+  if (TII->hasUnscaledLdStOffset(MI.getOpcode()))
+    return false;
+
+  // Look back to try to find a const offset for index LdSt instruction. For
+  // example,
+  // mov x8, #LargeImm   ; = a * (1<<12) + imm12
+  // ldr x1, [x0, x8]
+  // merged into:
+  // add x8, x0, a * (1<<12)
+  // ldr x1, [x8, imm12]
+  unsigned Offset;
+  Update = findMatchingConstOffsetBackward(MBBI, LdStConstLimit, Offset);
+  if (Update != E && (Offset & (Scale - 1)) == 0) {
+    // Merge the imm12 into the ld/st.
+    MBBI = mergeConstOffsetInsn(MBBI, Update, Offset, Scale);
+    return true;
+  }
+
+  return false;
+}
+
 bool AArch64LoadStoreOpt::optimizeBlock(MachineBasicBlock &MBB,
                                         bool EnableNarrowZeroStOpt) {
 
@@ -2512,6 +2728,22 @@ bool AArch64LoadStoreOpt::optimizeBlock(MachineBasicBlock &MBB,
       ++MBBI;
   }
 
+  // 5) Find a register assigned with a const value that can be combined with
+  // into the load or store. e.g.,
+  //        mov x8, #LargeImm   ; = a * (1<<12) + imm12
+  //        ldr x1, [x0, x8]
+  //        ; becomes
+  //        add x8, x0, a * (1<<12)
+  //        ldr x1, [x8, imm12]
+  for (MachineBasicBlock::iterator MBBI = MBB.begin(), E = MBB.end();
+       MBBI != E;) {
+    int Scale;
+    if (isMergeableIndexLdSt(*MBBI, Scale) && tryToMergeIndexLdSt(MBBI, Scale))
+      Modified = true;
+    else
+      ++MBBI;
+  }
+
   return Modified;
 }
 
diff --git a/llvm/test/CodeGen/AArch64/arm64-addrmode.ll b/llvm/test/CodeGen/AArch64/arm64-addrmode.ll
index d39029163a47a..2181eaaee7db6 100644
--- a/llvm/test/CodeGen/AArch64/arm64-addrmode.ll
+++ b/llvm/test/CodeGen/AArch64/arm64-addrmode.ll
@@ -214,9 +214,8 @@ define void @t17(i64 %a) {
 define i8 @LdOffset_i8(ptr %a)  {
 ; CHECK-LABEL: LdOffset_i8:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    mov w8, #56952 // =0xde78
-; CHECK-NEXT:    movk w8, #15, lsl #16
-; CHECK-NEXT:    ldrb w0, [x0, x8]
+; CHECK-NEXT:    add x8, x0, #253, lsl #12 // =1036288
+; CHECK-NEXT:    ldrb w0, [x8, #3704]
 ; CHECK-NEXT:    ret
   %arrayidx = getelementptr inbounds i8, ptr %a, i64 1039992
   %val = load i8, ptr %arrayidx, align 1
@@ -227,9 +226,8 @@ define i8 @LdOffset_i8(ptr %a)  {
 define i32 @LdOffset_i8_zext32(ptr %a)  {
 ; CHECK-LABEL: LdOffset_i8_zext32:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    mov w8, #56952 // =0xde78
-; CHECK-NEXT:    movk w8, #15, lsl #16
-; CHECK-NEXT:    ldrb w0, [x0, x8]
+; CHECK-NEXT:    add x8, x0, #253, lsl #12 // =1036288
+; CHECK-NEXT:    ldrb w0, [x8, #3704]
 ; CHECK-NEXT:    ret
   %arrayidx = getelementptr inbounds i8, ptr %a, i64 1039992
   %val = load i8, ptr %arrayidx, align 1
@@ -255,9 +253,8 @@ define i32 @LdOffset_i8_sext32(ptr %a)  {
 define i64 @LdOffset_i8_zext64(ptr %a)  {
 ; CHECK-LABEL: LdOffset_i8_zext64:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    mov w8, #56952 // =0xde78
-; CHECK-NEXT:    movk w8, #15, lsl #16
-; CHECK-NEXT:    ldrb w0, [x0, x8]
+; CHECK-NEXT:    add x8, x0, #253, lsl #12 // =1036288
+; CHECK-NEXT:    ldrb w0, [x8, #3704]
 ; CHECK-NEXT:    ret
   %arrayidx = getelementptr inbounds i8, ptr %a, i64 1039992
   %val = load i8, ptr %arrayidx, align 1
diff --git a/llvm/test/CodeGen/AArch64/large-offset-ldr-merge.mir b/llvm/test/CodeGen/AArch64/large-offset-ldr-merge.mir
index 488f1ffdb52f3..473c74323d939 100755
--- a/llvm/test/CodeGen/AArch64/large-offset-ldr-merge.mir
+++ b/llvm/test/CodeGen/AArch64/large-offset-ldr-merge.mir
@@ -14,9 +14,8 @@ body:             |
     ; CHECK-LABEL: name: LdOffset
     ; CHECK: liveins: $x0
     ; CHECK-NEXT: {{  $}}
-    ; CHECK-NEXT: renamable $w8 = MOVZWi 56952, 0
-    ; CHECK-NEXT: renamable $w8 = MOVKWi $w8, 15, 16, implicit-def $x8
-    ; CHECK-NEXT: renamable $w0 = LDRBBroX killed renamable $x0, killed renamable $x8, 0, 0
+    ; CHECK-NEXT: $x8 = ADDXri $x0, 253, 12
+    ; CHECK-NEXT: renamable $w0 = LDRBBui killed renamable $x8, 3704
     ; CHECK-NEXT: RET undef $lr, implicit $w0
     renamable $w8 = MOVZWi 56952, 0
     renamable $w8 = MOVKWi $w8, 15, 16, implicit-def $x8
@@ -46,3 +45,23 @@ body:             |
     renamable $w0 = LDRBBroX killed renamable $x0, renamable $x8, 0, 0
     RET undef $lr, implicit $w0
 ...
+
+# Negative test: No MOVZWi used for the const offset
+---
+name:            LdOffset_missing_MOVZ
+tracksRegLiveness: true
+liveins:
+  - { reg: '$x0', virtual-reg: '' }
+body:             |
+  bb.0.entry:
+    liveins: $x0
+
+    ; CHECK-LABEL: name: LdOffset_missing_MOVZ
+    ; CHECK: liveins: $x0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: renamable $w8 = MOVKWi $w8, 15, 16, implicit-def $x8
+    ; CHECK-NEXT: renamable $w0 = LDRBBroX killed renamable $x0, killed renamable $x8, 0, 0
+    ; CHECK-NEXT: RET undef $lr, implicit $w0
+    renamable $w8 = MOVKWi $w8, 15, 16, implicit-def $x8
+    renamable $w0 = LDRBBroX killed renamable $x0, killed renamable $x8, 0, 0
+    RET undef $lr, implicit $w0

@davemgreen
Copy link
Collaborator

Only LDRBBroX is supported for the first time.

I think all the instructions from f568763 should be added. Else we miss bugs due to a lack of test coverage.

@vfdff
Copy link
Contributor Author

vfdff commented Jan 30, 2024

Only LDRBBroX is supported for the first time.

I think all the instructions from f568763 should be added. Else we miss bugs due to a lack of test coverage.
Done, thanks

@davemgreen
Copy link
Collaborator

Thanks. Do you know if the issue from #76202 is still present, or what it might have been caused by? It sounded like a mis-compile.

@vfdff
Copy link
Contributor Author

vfdff commented Feb 1, 2024

Thanks. Do you know if the issue from #76202 is still present, or what it might have been caused by? It sounded like a mis-compile.

Oh, yes. I should also wait to figure out this issue, but now I don't have any idea.
I haven't had a smooth reproduction of the problem, and will take some more trying.

@vitalybuka
Copy link
Collaborator

Sorry for ignoring this patch?
Is this still relevant?
Do you need help with testing?

@vfdff
Copy link
Contributor Author

vfdff commented Aug 8, 2024

Yes, thank you very much for helping. I add the commit 46ea657 to fix a build crash, and this also fix a runtime issue for spec2017 with LTO enable after your bug report.
BTW: As this commit has noupdate for a long time, I'll rebase it today, I think it may be a little more convenient for you to test.

@vfdff vfdff force-pushed the fix-547-point2-fixPR79756 branch from 9c7f71f to 5b6d476 Compare August 8, 2024 04:03
@vfdff
Copy link
Contributor Author

vfdff commented Aug 12, 2024

hi @vitalybuka, I have finished the rebase, thank you in advance for your verification.

@vitalybuka
Copy link
Collaborator

vitalybuka commented Aug 12, 2024

I don't see how to validate the issue for sure, without rolling back bots to Ubuntu we used at that time.
I did a coupe of runs with your patches on the HEAD, and see no issue.
Let's try to re-land and watch bots, and we hit the issue, I will document and debug immediately.

@vfdff
Copy link
Contributor Author

vfdff commented Aug 13, 2024

I don't see how to validate the issue for sure, without rolling back bots to Ubuntu we used at that time.

Thanks very much for your time

@vfdff vfdff force-pushed the fix-547-point2-fixPR79756 branch from 5b6d476 to 63ce1b3 Compare August 15, 2024 10:21
vfdff added 2 commits August 15, 2024 18:22
A case for this transformation, https://gcc.godbolt.org/z/nhYcWq1WE
Fold
  mov     w8, llvm#56952
  movk    w8, llvm#15, lsl llvm#16
  ldrb    w0, [x0, x8]
into
  add     x0, x0, 1036288
  ldrb    w0, [x0, 3704]

Only LDRBBroX is supported for the first time.
Fix llvm#71917

Note: This PR is try relanding the commit 32878c2 with fix crash for PR79756
  this crash is exposes when there is MOVKWi instruction in the head of a block,
but without MOVZWi
The list of load.x is refer to canFoldIntoAddrMode on D152828.
Also support LDRSroX missed in canFoldIntoAddrMode
@vfdff vfdff force-pushed the fix-547-point2-fixPR79756 branch from 63ce1b3 to 43ffe2e Compare August 15, 2024 10:22
@vfdff vfdff merged commit 43ffe2e into llvm:main Aug 15, 2024
4 of 7 checks passed
@thurstond
Copy link
Contributor

FYI: I reverted 43ffe2e because of a buildbot breakage (324b676)

@thurstond
Copy link
Contributor

Since buildbot output expires, here is the failure message for future reference:

FAIL: LLVM :: CodeGen/PowerPC/ppc64-nest.ll (38035 of 81639)
******************** TEST 'LLVM :: CodeGen/PowerPC/ppc64-nest.ll' FAILED ********************
Exit Code: 2
Command Output (stderr):
--
RUN: at line 1: /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/llc -verify-machineinstrs < /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/test/CodeGen/PowerPC/ppc64-nest.ll | /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/FileCheck /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/test/CodeGen/PowerPC/ppc64-nest.ll
+ /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/llc -verify-machineinstrs
+ /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/FileCheck /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/test/CodeGen/PowerPC/ppc64-nest.ll
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/llc -verify-machineinstrs
1.	Running pass 'Function Pass Manager' on module '<stdin>'.
2.	Running pass 'PowerPC DAG->DAG Pattern Instruction Selection' on function '@nest_caller'
 #0 0x0000b078769c0bc0 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Support/Unix/Signals.inc:723:13
 #1 0x0000b078769bdcd8 __cxx_atomic_store<CallbackAndCookie::Status> /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/libcxx_build_ubsan/include/c++/v1/__atomic/cxx_atomic_impl.h:304:3
 #2 0x0000b078769bdcd8 store /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/libcxx_build_ubsan/include/c++/v1/__atomic/atomic_base.h:51:5
 #3 0x0000b078769bdcd8 llvm::sys::RunSignalHandlers() /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Support/Signals.cpp:107:16
 #4 0x0000b078769c16ac SignalHandler(int) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Support/Unix/Signals.inc:413:1
 #5 0x0000efaa9b33b8f8 (linux-vdso.so.1+0x8f8)
 #6 0x0000b07874395294 getOS /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/TargetParser/Triple.h:382:33
 #7 0x0000b07874395294 isOSAIX /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/TargetParser/Triple.h:711:12
 #8 0x0000b07874395294 isAIXABI /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Target/PowerPC/PPCSubtarget.h:215:47
 #9 0x0000b07874395294 llvm::PPCTargetLowering::LowerCall(llvm::TargetLowering::CallLoweringInfo&, llvm::SmallVectorImpl<llvm::SDValue>&) const /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5971:17
#10 0x0000b0787666cc48 llvm::TargetLowering::LowerCallTo(llvm::TargetLowering::CallLoweringInfo&) const /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:0:15
#11 0x0000b0787668ddd0 llvm::SelectionDAGBuilder::lowerInvokable(llvm::TargetLowering::CallLoweringInfo&, llvm::BasicBlock const*) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8700:3
#12 0x0000b078766751e4 getNode /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/CodeGen/SelectionDAGNodes.h:159:36
#13 0x0000b078766751e4 llvm::SelectionDAGBuilder::LowerCallTo(llvm::CallBase const&, llvm::SDValue, bool, bool, llvm::BasicBlock const*, llvm::TargetLowering::PtrAuthInfo const*) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8847:20
#14 0x0000b0787665bb74 llvm::SelectionDAGBuilder::visitCall(llvm::CallInst const&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9467:1
#15 0x0000b0787664e7e0 getValueID /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/IR/Value.h:533:12
#16 0x0000b0787664e7e0 getOpcode /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/IR/Instruction.h:274:39
#17 0x0000b0787664e7e0 isTerminator /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/IR/Instruction.h:277:51
#18 0x0000b0787664e7e0 llvm::SelectionDAGBuilder::visit(llvm::Instruction const&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1346:10
#19 0x0000b0787674366c getNext /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/ADT/ilist_node_base.h:42:38
#20 0x0000b0787674366c getNext /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/ADT/ilist_node.h:125:59
#21 0x0000b0787674366c operator++ /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/ADT/ilist_iterator.h:345:57
#22 0x0000b0787674366c llvm::SelectionDAGISel::SelectBasicBlock(llvm::ilist_iterator_w_bits<llvm::ilist_detail::node_options<llvm::Instruction, true, false, void, true, llvm::BasicBlock>, false, true>, llvm::ilist_iterator_w_bits<llvm::ilist_detail::node_options<llvm::Instruction, true, false, void, true, llvm::BasicBlock>, false, true>, bool&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:816:77
#23 0x0000b07876742094 llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:0:7
#24 0x0000b0787673e988 llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:615:7
#25 0x0000b078743e7c50 (anonymous namespace)::PPCDAGToDAGISel::runOnMachineFunction(llvm::MachineFunction&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:171:7
#26 0x0000b0787673bf28 llvm::SelectionDAGISelLegacy::runOnMachineFunction(llvm::MachineFunction&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:374:20
#27 0x0000b0787560d958 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/MachineFunctionPass.cpp:94:13
#28 0x0000b07875d0b078 llvm::FPPassManager::runOnFunction(llvm::Function&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:0:27
#29 0x0000b07875d14e44 getNext /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/ADT/ilist_node_base.h:42:38
#30 0x0000b07875d14e44 getNext /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/ADT/ilist_node.h:117:59
#31 0x0000b07875d14e44 operator++ /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/ADT/ilist_iterator.h:187:57
#32 0x0000b07875d14e44 llvm::FPPassManager::runOnModule(llvm::Module&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1453:20
#33 0x0000b07875d0bb38 runOnModule /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:0:27
#34 0x0000b07875d0bb38 llvm::legacy::PassManagerImpl::run(llvm::Module&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:541:44
#35 0x0000b0787309d90c compileModule(char**, llvm::LLVMContext&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/tools/llc/llc.cpp:741:17
#36 0x0000b0787309b2f0 main /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/tools/llc/llc.cpp:0:22
#37 0x0000efaa9acc84c4 (/lib/aarch64-linux-gnu/libc.so.6+0x284c4)
#38 0x0000efaa9acc8598 __libc_start_main (/lib/aarch64-linux-gnu/libc.so.6+0x28598)
#39 0x0000b0787306dc30 _start (/home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/llc+0x68ddc30)
UndefinedBehaviorSanitizer:DEADLYSIGNAL
==1541260==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x220022002324 (pc 0xb07874395294 bp 0xffffede79610 sp 0xffffede795b0 T1541260)
==1541260==The signal is caused by a READ memory access.
    #0 0xb07874395294 in getOS /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/TargetParser/Triple.h:382:33
    #1 0xb07874395294 in isOSAIX /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/TargetParser/Triple.h:711:12
    #2 0xb07874395294 in isAIXABI /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Target/PowerPC/PPCSubtarget.h:215:47
    #3 0xb07874395294 in llvm::PPCTargetLowering::LowerCall(llvm::TargetLowering::CallLoweringInfo&, llvm::SmallVectorImpl<llvm::SDValue>&) const /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5971:17
    #4 0xb0787666cc44 in llvm::TargetLowering::LowerCallTo(llvm::TargetLowering::CallLoweringInfo&) const /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:11113:15
    #5 0xb0787668ddcc in llvm::SelectionDAGBuilder::lowerInvokable(llvm::TargetLowering::CallLoweringInfo&, llvm::BasicBlock const*) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8698:44
    #6 0xb078766751e0 in llvm::SelectionDAGBuilder::LowerCallTo(llvm::CallBase const&, llvm::SDValue, bool, bool, llvm::BasicBlock const*, llvm::TargetLowering::PtrAuthInfo const*) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8845:40
    #7 0xb0787665bb70 in llvm::SelectionDAGBuilder::visitCall(llvm::CallInst const&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9466:5
    #8 0xb0787664e7dc in llvm::SelectionDAGBuilder::visit(llvm::Instruction const&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1344:3
    #9 0xb07876743668 in llvm::SelectionDAGISel::SelectBasicBlock(llvm::ilist_iterator_w_bits<llvm::ilist_detail::node_options<llvm::Instruction, true, false, void, true, llvm::BasicBlock>, false, true>, llvm::ilist_iterator_w_bits<llvm::ilist_detail::node_options<llvm::Instruction, true, false, void, true, llvm::BasicBlock>, false, true>, bool&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:818:12
    #10 0xb07876742090 in llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1839:7
    #11 0xb0787673e984 in llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:614:3
    #12 0xb078743e7c4c in (anonymous namespace)::PPCDAGToDAGISel::runOnMachineFunction(llvm::MachineFunction&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:169:25
    #13 0xb0787673bf24 in llvm::SelectionDAGISelLegacy::runOnMachineFunction(llvm::MachineFunction&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:374:20
    #14 0xb0787560d954 in llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/MachineFunctionPass.cpp:94:13
    #15 0xb07875d0b074 in llvm::FPPassManager::runOnFunction(llvm::Function&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1408:27
    #16 0xb07875d14e40 in llvm::FPPassManager::runOnModule(llvm::Module&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1454:16
    #17 0xb07875d0bb34 in runOnModule /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1523:27
    #18 0xb07875d0bb34 in llvm::legacy::PassManagerImpl::run(llvm::Module&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:541:44
    #19 0xb0787309d908 in compileModule(char**, llvm::LLVMContext&) /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/tools/llc/llc.cpp:739:8
    #20 0xb0787309b2ec in main /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/tools/llc/llc.cpp:408:22
    #21 0xefaa9acc84c0  (/lib/aarch64-linux-gnu/libc.so.6+0x284c0) (BuildId: 3119d7bcf68a34e21742cd08095272de601d373f)
    #22 0xefaa9acc8594 in __libc_start_main (/lib/aarch64-linux-gnu/libc.so.6+0x28594) (BuildId: 3119d7bcf68a34e21742cd08095272de601d373f)
    #23 0xb0787306dc2c in _start (/home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/llc+0x68ddc2c)
==1541260==Register values:
 x0 = 0x2200220022002200   x1 = 0x0000ffffede79be0   x2 = 0x0000ffffede79c18   x3 = 0x0000b078b1a6ccd0  
 x4 = 0x0000000000000000   x5 = 0x0000ffffede79848   x6 = 0x0000000000000001   x7 = 0x0000000000000008  
 x8 = 0x0000000000000038   x9 = 0x0000000000000000  x10 = 0x0000000000000001  x11 = 0x0000efaa9ac2d670  
x12 = 0x0000000000000000  x13 = 0x0000000000000000  x14 = 0x0000000000000000  x15 = 0x0000b078b19f9890  
x16 = 0x0000000000000001  x17 = 0x0000000000000000  x18 = 0x0000000000000001  x19 = 0x0000efaa9abed670  
x20 = 0x0000ffffede79b60  x21 = 0x0000000000000000  x22 = 0x0000b078b1a13500  x23 = 0x0000b078b1a6cbf0  
x24 = 0x0000000000000000  x25 = 0x0000000000000000  x26 = 0x0000b07800000000  x27 = 0x0000b078b1a13500  
x28 = 0x0000b078b1a135f0   fp = 0x0000ffffede79610   lr = 0x0000b07874395204   sp = 0x0000ffffede795b0  
UndefinedBehaviorSanitizer can not provide additional info.
SUMMARY: UndefinedBehaviorSanitizer: SEGV /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/include/llvm/TargetParser/Triple.h:382:33 in getOS
==1541260==ABORTING
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/FileCheck /home/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/llvm-project/llvm/test/CodeGen/PowerPC/ppc64-nest.ll

It might be related to #76202

@vfdff
Copy link
Contributor Author

vfdff commented Aug 17, 2024

FYI: I reverted 43ffe2e because of a buildbot breakage (324b676)

Thanks for your report, I'll take a look at it .would you please show how to build the above llvm ? My change doesn't touch the ppc target, and it is a little supprise to affact the ppc64-nest.ll under target PowerPC.

@thurstond
Copy link
Contributor

thurstond commented Aug 17, 2024

FYI: I reverted 43ffe2e because of a buildbot breakage (324b676)

Thanks for your report, I'll take a look at it .would you please show how to build the above llvm ? My change doesn't touch the ppc target, and it is a little supprise to affact the ppc64-nest.ll under target PowerPC.

The crash is in the clang compiler running on aarch64 (not PPC) Linux while compiling the ppc64-nest.ll, so it's just a coincidence that the input file is ppc64-.

Here's the repro instructions:

mkdir scratch_dir
cd scratch_dir
git clone https://github.com/llvm/llvm-zorg.git
BUILDBOT_REVISION=43ffe2eed0d9f73789dbe213023733d164999306 llvm-zorg/zorg/buildbot/builders/sanitizers/buildbot_bootstrap_ubsan.sh

(based on https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild but with your commit revision and the ubsan script)

Thanks!

@vfdff
Copy link
Contributor Author

vfdff commented Aug 20, 2024

hi , thurstond
I try to reproduce the issue with your instructions locally, and I don't see obvious build error information, but only get a binary llvm_build0/bin/llc. but llvm_build0/bin/llc -verify-machineinstrs llvm-project/llvm/test/CodeGen/PowerPC/ppc64-nest.ll doesn't crash on my aarch64 machine. I suspect my local compilation is incomplete for some reason ?

~/source/scratch_dir » find . -name "llc"
./llvm_build_ubsan/tools/llc
./llvm_build0/bin/llc
./llvm_build0/tools/llc
------------------------------------------------------------
~/source/scratch_dir » ls
build.log  libcxx_build_ubsan  llvm_build0  llvm_build_ubsan  llvm-project  llvm-zorg

@thurstond
Copy link
Contributor

hi , thurstond I try to reproduce the issue with your instructions locally, and I don't see obvious build error information, but only get a binary llvm_build0/bin/llc. but llvm_build0/bin/llc -verify-machineinstrs llvm-project/llvm/test/CodeGen/PowerPC/ppc64-nest.ll doesn't crash on my aarch64 machine. I suspect my local compilation is incomplete for some reason ?

~/source/scratch_dir » find . -name "llc"
./llvm_build_ubsan/tools/llc
./llvm_build0/bin/llc
./llvm_build0/tools/llc
------------------------------------------------------------
~/source/scratch_dir » ls
build.log  libcxx_build_ubsan  llvm_build0  llvm_build_ubsan  llvm-project  llvm-zorg

The failure previously happened in "stage2/ubsan" (https://lab.llvm.org/buildbot/#/builders/85/builds/1102) i.e., using llvm_build_ubsan/bin/llc. I don't know why that llc wasn't built. The script is supposed to automatically build it and run the tests.

Are you out of disk space by any chance?

If not, could you please paste all the output from BUILDBOT_REVISION=43ffe2eed0d9f73789dbe213023733d164999306 llvm-zorg/zorg/buildbot/builders/sanitizers/buildbot_bootstrap_ubsan.sh into a gist or similar?

@thurstond
Copy link
Contributor

P.S. the error on the buildbots was consistently

==1541260==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x220022002324 (pc 0xb07874395294 bp 0xffffede79610 sp 0xffffede795b0 T1541260)

e.g., https://lab.llvm.org/buildbot/#/builders/85/builds/1102, https://lab.llvm.org/buildbot/#/builders/85/builds/1103, ..., https://lab.llvm.org/buildbot/#/builders/85/builds/1125

Address space layout randomization is enabled on the bots, so it's exceedingly unlikely to get the same memory address by chance. This suggests that, somehow, the pointer is being overwritten by some data.

@vfdff
Copy link
Contributor Author

vfdff commented Aug 22, 2024

Thanks @thurstond, I can reproduce with llvm_build_ubsan/bin/llc now. I check the build log and find missing /usr/bin/time , therefore, the build was not completed before

llvm_build_ubsan/bin/llc -verify-machineinstrs llvm-project/llvm/test/CodeGen/PowerPC/ppc64-nest.ll

@vfdff
Copy link
Contributor Author

vfdff commented Aug 23, 2024

  • I am surprised that it does affect the back end of the powerpc64. also simplified test base on above ppc64-nest.ll, https://gcc.godbolt.org/z/YdG4r3vcb

    get same issue with : llvm_build_ubsan/bin/llc --mtriple powerpc64-unknown-linux-gnu test.ll
    works fine with: llvm_build_ubsan/bin/llc --mtriple aarch64-unknown-linux-gnu test.ll

@vfdff
Copy link
Contributor Author

vfdff commented Aug 26, 2024

  • I find the root cause: it can only be reproduce when configure the build with -DCMAKE_BUILD_TYPE=Release, so I dump all the matched scenarios ,and find the Release version generate extra case compare to -DCMAKE_BUILD_TYPE=RelWithDebInfo, where MOVZWi and MOVKWi don't set the same registers, so should add checking to avoid this case.

    CMAKE_COMMON_OPTIONS+=" -DLLVM_APPEND_VC_REV=OFF -GNinja -DCMAKE_BUILD_TYPE=Release"

Offset:  262160  Scale:  8
Creating base address load/store.
    Replacing instructions:
    renamable $w8 = MOVZWi 16, 0
    renamable $w11 = MOVKWi killed $w11(tied-def 0), 4, 16, implicit-def $x11
    renamable $x0 = LDRXroX renamable $x19, killed renamable $x11, 0, 0, debug-location !16303 :: (dereferenceable load (s64) from %ir.Subtarget163, !tbaa !1062); llvm-project/llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5971:7
  with instruction:
    $x11 = ADDXri $x19, 64, 12, debug-location !16303; llvm-project/llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5971:7
    renamable $x0 = LDRXui killed renamable $x11, 2, debug-location !16303 :: (dereferenceable load (s64) from %ir.Subtarget163, !tbaa !1062); llvm-project/llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5971:7

@thurstond
Copy link
Contributor

Nice work Allen! Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Miscompile after f5687636415969e6d945659a0b78734abdfb0f06 [AArch64] Optimize the offset of memory access
5 participants