Skip to content

Conversation

@phoebewang
Copy link
Contributor

3rd try to fix compile regression by #113532

@phoebewang phoebewang requested a review from nikic November 5, 2024 07:12
@llvmbot
Copy link
Member

llvmbot commented Nov 5, 2024

@llvm/pr-subscribers-debuginfo

Author: Phoebe Wang (phoebewang)

Changes

3rd try to fix compile regression by #113532


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

2 Files Affected:

  • (modified) llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp (+12-8)
  • (modified) llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h (+2)
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
index a9d28a39c4418b..d99c8039aa3497 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
+++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
@@ -1022,7 +1022,7 @@ MLocTracker::MLocTracker(MachineFunction &MF, const TargetInstrInfo &TII,
                          const TargetLowering &TLI)
     : MF(MF), TII(TII), TRI(TRI), TLI(TLI),
       LocIdxToIDNum(ValueIDNum::EmptyValue), LocIdxToLocID(0) {
-  NumRegs = TRI.getNumRegs();
+  NumRegs = TRI.getNumSupportedRegs(MF);
   reset();
   LocIDToLocIdx.resize(NumRegs, LocIdx::MakeIllegalLoc());
   assert(NumRegs < (1u << NUM_LOC_BITS)); // Detect bit packing failure
@@ -1878,7 +1878,8 @@ void InstrRefBasedLDV::transferRegisterDef(MachineInstr &MI) {
     if (MO.isReg() && MO.isDef() && MO.getReg() && MO.getReg().isPhysical() &&
         !IgnoreSPAlias(MO.getReg())) {
       // Remove ranges of all aliased registers.
-      for (MCRegAliasIterator RAI(MO.getReg(), TRI, true); RAI.isValid(); ++RAI)
+      for (MCRegAliasIterator RAI(MO.getReg(), TRI, true);
+           RAI.isValid() && *RAI < NumRegs; ++RAI)
         // FIXME: Can we break out of this loop early if no insertion occurs?
         DeadRegs.insert(*RAI);
     } else if (MO.isRegMask()) {
@@ -1952,7 +1953,8 @@ void InstrRefBasedLDV::transferRegisterDef(MachineInstr &MI) {
 
 void InstrRefBasedLDV::performCopy(Register SrcRegNum, Register DstRegNum) {
   // In all circumstances, re-def all aliases. It's definitely a new value now.
-  for (MCRegAliasIterator RAI(DstRegNum, TRI, true); RAI.isValid(); ++RAI)
+  for (MCRegAliasIterator RAI(DstRegNum, TRI, true);
+       RAI.isValid() && *RAI < NumRegs; ++RAI)
     MTracker->defReg(*RAI, CurBB, CurInst);
 
   ValueIDNum SrcValue = MTracker->readReg(SrcRegNum);
@@ -2117,7 +2119,8 @@ bool InstrRefBasedLDV::transferSpillOrRestoreInst(MachineInstr &MI) {
     // stack slot.
 
     // Def all registers that alias the destination.
-    for (MCRegAliasIterator RAI(Reg, TRI, true); RAI.isValid(); ++RAI)
+    for (MCRegAliasIterator RAI(Reg, TRI, true);
+         RAI.isValid() && *RAI < NumRegs; ++RAI)
       MTracker->defReg(*RAI, CurBB, CurInst);
 
     // Now find subregisters within the destination register, and load values
@@ -2302,11 +2305,12 @@ void InstrRefBasedLDV::produceMLocTransferFunction(
   // appropriate clobbers.
   SmallVector<BitVector, 32> BlockMasks;
   BlockMasks.resize(MaxNumBlocks);
+  NumRegs = TRI->getNumSupportedRegs(MF);
 
   // Reserve one bit per register for the masks described above.
-  unsigned BVWords = MachineOperand::getRegMaskSize(TRI->getNumRegs());
+  unsigned BVWords = MachineOperand::getRegMaskSize(NumRegs);
   for (auto &BV : BlockMasks)
-    BV.resize(TRI->getNumRegs(), true);
+    BV.resize(NumRegs, true);
 
   // Step through all instructions and inhale the transfer function.
   for (auto &MBB : MF) {
@@ -2370,11 +2374,11 @@ void InstrRefBasedLDV::produceMLocTransferFunction(
   }
 
   // Compute a bitvector of all the registers that are tracked in this block.
-  BitVector UsedRegs(TRI->getNumRegs());
+  BitVector UsedRegs(NumRegs);
   for (auto Location : MTracker->locations()) {
     unsigned ID = MTracker->LocIdxToLocID[Location.Idx];
     // Ignore stack slots, and aliases of the stack pointer.
-    if (ID >= TRI->getNumRegs() || MTracker->SPAliases.count(ID))
+    if (ID >= NumRegs || MTracker->SPAliases.count(ID))
       continue;
     UsedRegs.set(ID);
   }
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h
index 68db65ace9a427..cdb9da236a84e4 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h
+++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h
@@ -1253,6 +1253,8 @@ class InstrRefBasedLDV : public LDVImpl {
   /// pointer.
   StringRef StackProbeSymbolName;
 
+  unsigned NumRegs = 0;
+
   /// Tests whether this instruction is a spill to a stack slot.
   std::optional<SpillLocationNo> isSpillInstruction(const MachineInstr &MI,
                                                     MachineFunction *MF);

@phoebewang
Copy link
Contributor Author

Verified with valgrind with below command
vg-in-place --tool=callgrind clang -DNDEBUG -O3 -fomit-frame-pointer -DNDEBUG -g -w -Werror=date-time -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DSQLITE_OMIT_LOAD_EXTENSION=1 -DSQLITE_THREADSAFE=0 -I. -MD -MT shell.c.o -MF shell.c.o.d -o shell.c.o -c MultiSource/Applications/sqlite3/shell.c

Patches Instructions Ratio
Without #113532 1790650933 -
With #113532 1793289982 +0.15%
With both 1790138180 -0.03%

@phoebewang
Copy link
Contributor Author

Need to investigate lit failures.

@jmorse
Copy link
Member

jmorse commented Nov 5, 2024

(While this is marked draft, reducing the known set of register that might need tracking is a solid plan, so this is a good direction to take).

@nikic
Copy link
Contributor

nikic commented Nov 5, 2024

I wasn't able to confirm the compile-time impact because the LTO build crashed: https://llvm-compile-time-tracker.com/show_error.php?commit=428bd77c9a8807b3780d39ad5b5a757ac65dfaef (Presumably related to the lit test failures.)

@github-actions
Copy link

github-actions bot commented Nov 5, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@phoebewang
Copy link
Contributor Author

The crash issue should be solved, but there's still a lit failure: llvm/test/CodeGen/X86/debuginfo-locations-dce.ll
With this patch, we lack the information of DW_AT_location in

0x00000043:     DW_TAG_variable
                  DW_AT_name   ("MyVar")
                  DW_AT_decl_file      ("./test.cpp")
                  DW_AT_decl_line      (4)
                  DW_AT_type   (0x00000056 "float")

I know little about the pass, so I have difficulty to figure out what's wrong here. I wouldn't expect there's any functional change with this patch.

@nikic
Copy link
Contributor

nikic commented Nov 5, 2024

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.

4 participants