Skip to content

[LiveDebugValues] Use getNumSupportedRegs to reduce compile time, NFCI #114944

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

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.)

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