Merge sourcelocation in CSEMIRBuilder::getDominatingInstrForID. #90922
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-globalisel Author: Shubham Sandeep Rastogi (rastogishubham) ChangesMake sure to merge the sourcelocation of the Dominating Instruction that is hoisted in a basic block in the CSEMIRBuilder in the legalizer pass. If this is not done, we can have a incorrect line table entry that makes the instruction pointer jump around. For example the line table without this patch looks like: The line table entry for 0x000000000000005c should be 0 After this patch, the line table looks like: Full diff: https://github.com/llvm/llvm-project/pull/90922.diff 6 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/CSEMIRBuilder.h b/llvm/include/llvm/CodeGen/GlobalISel/CSEMIRBuilder.h
index 488df12f13f8df..78341fa1b47bc9 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/CSEMIRBuilder.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/CSEMIRBuilder.h
@@ -47,8 +47,9 @@ class CSEMIRBuilder : public MachineIRBuilder {
/// dominates the current insertion point and if not, move it just before the
/// current insertion point and return it. If not found, return Null
/// MachineInstrBuilder.
- MachineInstrBuilder getDominatingInstrForID(FoldingSetNodeID &ID,
- void *&NodeInsertPos);
+ MachineInstrBuilder
+ getDominatingInstrForID(FoldingSetNodeID &ID, void *&NodeInsertPos,
+ SmallVectorImpl<DILocation *> *Locs = nullptr);
/// Simple check if we can CSE (we have the CSEInfo) or if this Opcode is
/// safe to CSE.
bool canPerformCSEForOpc(unsigned Opc) const;
@@ -97,8 +98,9 @@ class CSEMIRBuilder : public MachineIRBuilder {
// Bring in the other overload from the base class.
using MachineIRBuilder::buildConstant;
- MachineInstrBuilder buildConstant(const DstOp &Res,
- const ConstantInt &Val) override;
+ MachineInstrBuilder
+ buildConstant(const DstOp &Res, const ConstantInt &Val,
+ SmallVectorImpl<DILocation *> *Locs = nullptr) override;
// Bring in the other overload from the base class.
using MachineIRBuilder::buildFConstant;
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h b/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
index 305bef7dd3ea63..266bdb80614f01 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
@@ -99,8 +99,12 @@ class LegalizationArtifactCombiner {
const LLT DstTy = MRI.getType(DstReg);
if (isInstLegal({TargetOpcode::G_CONSTANT, {DstTy}})) {
auto &CstVal = SrcMI->getOperand(1);
+ SmallVector<DILocation *> Locs;
+ Locs.push_back(MI.getDebugLoc().get());
+ Locs.push_back(SrcMI->getDebugLoc().get());
Builder.buildConstant(
- DstReg, CstVal.getCImm()->getValue().sext(DstTy.getSizeInBits()));
+ DstReg, CstVal.getCImm()->getValue().sext(DstTy.getSizeInBits()),
+ &Locs);
UpdatedDefs.push_back(DstReg);
markInstAndDefDead(MI, *SrcMI, DeadInsts);
return true;
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
index e15f7a7172e1a4..dc67a3321a0d9d 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
@@ -856,8 +856,9 @@ class MachineIRBuilder {
/// type.
///
/// \return The newly created instruction.
- virtual MachineInstrBuilder buildConstant(const DstOp &Res,
- const ConstantInt &Val);
+ virtual MachineInstrBuilder
+ buildConstant(const DstOp &Res, const ConstantInt &Val,
+ SmallVectorImpl<DILocation *> *Locs = nullptr);
/// Build and insert \p Res = G_CONSTANT \p Val
///
@@ -868,7 +869,9 @@ class MachineIRBuilder {
///
/// \return The newly created instruction.
MachineInstrBuilder buildConstant(const DstOp &Res, int64_t Val);
- MachineInstrBuilder buildConstant(const DstOp &Res, const APInt &Val);
+ MachineInstrBuilder
+ buildConstant(const DstOp &Res, const APInt &Val,
+ SmallVectorImpl<DILocation *> *Locs = nullptr);
/// Build and insert \p Res = G_FCONSTANT \p Val
///
diff --git a/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp b/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
index 551ba1e6036c17..893bebf4da327d 100644
--- a/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
@@ -36,7 +36,8 @@ bool CSEMIRBuilder::dominates(MachineBasicBlock::const_iterator A,
MachineInstrBuilder
CSEMIRBuilder::getDominatingInstrForID(FoldingSetNodeID &ID,
- void *&NodeInsertPos) {
+ void *&NodeInsertPos,
+ SmallVectorImpl<DILocation *> *Locs) {
GISelCSEInfo *CSEInfo = getCSEInfo();
assert(CSEInfo && "Can't get here without setting CSEInfo");
MachineBasicBlock *CurMBB = &getMBB();
@@ -51,6 +52,11 @@ CSEMIRBuilder::getDominatingInstrForID(FoldingSetNodeID &ID,
// this builder will have the def ready.
setInsertPt(*CurMBB, std::next(MII));
} else if (!dominates(MI, CurrPos)) {
+ if (Locs) {
+ Locs->push_back(MI->getDebugLoc().get());
+ auto *Loc = DILocation::getMergedLocations(*Locs);
+ MI->setDebugLoc(Loc);
+ }
CurMBB->splice(CurrPos, CurMBB, MI);
}
return MachineInstrBuilder(getMF(), MI);
@@ -320,8 +326,9 @@ MachineInstrBuilder CSEMIRBuilder::buildInstr(unsigned Opc,
return memoizeMI(NewMIB, InsertPos);
}
-MachineInstrBuilder CSEMIRBuilder::buildConstant(const DstOp &Res,
- const ConstantInt &Val) {
+MachineInstrBuilder
+CSEMIRBuilder::buildConstant(const DstOp &Res, const ConstantInt &Val,
+ SmallVectorImpl<DILocation *> *Locs) {
constexpr unsigned Opc = TargetOpcode::G_CONSTANT;
if (!canPerformCSEForOpc(Opc))
return MachineIRBuilder::buildConstant(Res, Val);
@@ -337,7 +344,7 @@ MachineInstrBuilder CSEMIRBuilder::buildConstant(const DstOp &Res,
profileMBBOpcode(ProfBuilder, Opc);
profileDstOp(Res, ProfBuilder);
ProfBuilder.addNodeIDMachineOperand(MachineOperand::CreateCImm(&Val));
- MachineInstrBuilder MIB = getDominatingInstrForID(ID, InsertPos);
+ MachineInstrBuilder MIB = getDominatingInstrForID(ID, InsertPos, Locs);
if (MIB) {
// Handle generating copies here.
return generateCopiesIfRequired({Res}, MIB);
diff --git a/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp b/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
index 2e8407813ba641..5cbdfe6b531dff 100644
--- a/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
@@ -314,8 +314,9 @@ MachineInstrBuilder MachineIRBuilder::buildCopy(const DstOp &Res,
return buildInstr(TargetOpcode::COPY, Res, Op);
}
-MachineInstrBuilder MachineIRBuilder::buildConstant(const DstOp &Res,
- const ConstantInt &Val) {
+MachineInstrBuilder
+MachineIRBuilder::buildConstant(const DstOp &Res, const ConstantInt &Val,
+ SmallVectorImpl<DILocation *> *Locs) {
LLT Ty = Res.getLLTTy(*getMRI());
LLT EltTy = Ty.getScalarType();
assert(EltTy.getScalarSizeInBits() == Val.getBitWidth() &&
@@ -375,10 +376,11 @@ MachineInstrBuilder MachineIRBuilder::buildFConstant(const DstOp &Res,
return Const;
}
-MachineInstrBuilder MachineIRBuilder::buildConstant(const DstOp &Res,
- const APInt &Val) {
+MachineInstrBuilder
+MachineIRBuilder::buildConstant(const DstOp &Res, const APInt &Val,
+ SmallVectorImpl<DILocation *> *Locs) {
ConstantInt *CI = ConstantInt::get(getMF().getFunction().getContext(), Val);
- return buildConstant(Res, *CI);
+ return buildConstant(Res, *CI, Locs);
}
MachineInstrBuilder MachineIRBuilder::buildFConstant(const DstOp &Res,
diff --git a/llvm/test/DebugInfo/merge-locations-legalizer.mir b/llvm/test/DebugInfo/merge-locations-legalizer.mir
new file mode 100644
index 00000000000000..92bd5de2858e69
--- /dev/null
+++ b/llvm/test/DebugInfo/merge-locations-legalizer.mir
@@ -0,0 +1,58 @@
+# RUN: llc %s -O0 --start-before=legalizer --stop-after=legalizer -o - | FileCheck %s
+# CHECK-NOT: %35:_(s32) = G_CONSTANT i32 0, debug-location !71
+# CHECK: %35:_(s32) = G_CONSTANT i32 0, debug-location !DILocation(line: 0,
+--- |
+ define i32 @main(i32 %0, ptr %1) #0 !dbg !57 {
+ entry:
+ ret i32 0, !dbg !71
+ }
+ !3 = !DIFile(filename: "/Volumes/Data/swift/llvm-project/lldb/test/API/lang/swift/external_provider_extra_inhabitants/main.swift", directory: "/Volumes/Data/swift")
+ !23 = distinct !DICompileUnit(language: DW_LANG_Swift, file: !3, sdk: "MacOSX14.2.sdk")
+ !57 = distinct !DISubprogram(name: "main", unit: !23)
+ !64 = distinct !DILexicalBlock(scope: !57, column: 1)
+ !66 = distinct !DILexicalBlock(scope: !64, column: 1)
+ !68 = !DILocation(line: 12, scope: !66)
+ !70 = distinct !DILexicalBlock(scope: !66, column: 1)
+ !71 = !DILocation(line: 13, scope: !70)
+name: main
+registers:
+ - { id: 0, class: _, preferred-register: '' }
+ - { id: 1, class: _, preferred-register: '' }
+ - { id: 2, class: _, preferred-register: '' }
+ - { id: 3, class: _, preferred-register: '' }
+ - { id: 4, class: _, preferred-register: '' }
+ - { id: 5, class: _, preferred-register: '' }
+ - { id: 6, class: _, preferred-register: '' }
+ - { id: 7, class: _, preferred-register: '' }
+ - { id: 8, class: _, preferred-register: '' }
+ - { id: 9, class: _, preferred-register: '' }
+ - { id: 10, class: _, preferred-register: '' }
+ - { id: 11, class: _, preferred-register: '' }
+ - { id: 12, class: _, preferred-register: '' }
+ - { id: 13, class: _, preferred-register: '' }
+ - { id: 14, class: _, preferred-register: '' }
+ - { id: 15, class: gpr64, preferred-register: '' }
+ - { id: 18, class: _, preferred-register: '' }
+ - { id: 19, class: _, preferred-register: '' }
+ - { id: 20, class: _, preferred-register: '' }
+ - { id: 21, class: _, preferred-register: '' }
+ - { id: 22, class: _, preferred-register: '' }
+ - { id: 23, class: _, preferred-register: '' }
+ - { id: 24, class: _, preferred-register: '' }
+ - { id: 25, class: _, preferred-register: '' }
+ - { id: 26, class: _, preferred-register: '' }
+ - { id: 27, class: _, preferred-register: '' }
+ - { id: 28, class: _, preferred-register: '' }
+ - { id: 29, class: _, preferred-register: '' }
+ - { id: 30, class: _, preferred-register: '' }
+ - { id: 31, class: _, preferred-register: '' }
+ - { id: 32, class: _, preferred-register: '' }
+ - { id: 33, class: _, preferred-register: '' }
+ - { id: 34, class: _, preferred-register: '' }
+body: |
+ bb.1.entry:
+ %16:_(s8) = G_CONSTANT i8 0, debug-location !68
+ %17:_(s32) = G_ANYEXT %16(s8), debug-location !68
+ $w2 = COPY %17(s32), debug-location !68
+ %35:_(s32) = G_CONSTANT i32 0, debug-location !71
+ $w0 = COPY %35(s32), debug-location !71
|
|
@llvm/pr-subscribers-debuginfo Author: Shubham Sandeep Rastogi (rastogishubham) ChangesMake sure to merge the sourcelocation of the Dominating Instruction that is hoisted in a basic block in the CSEMIRBuilder in the legalizer pass. If this is not done, we can have a incorrect line table entry that makes the instruction pointer jump around. For example the line table without this patch looks like: The line table entry for 0x000000000000005c should be 0 After this patch, the line table looks like: Full diff: https://github.com/llvm/llvm-project/pull/90922.diff 6 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/CSEMIRBuilder.h b/llvm/include/llvm/CodeGen/GlobalISel/CSEMIRBuilder.h
index 488df12f13f8df..78341fa1b47bc9 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/CSEMIRBuilder.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/CSEMIRBuilder.h
@@ -47,8 +47,9 @@ class CSEMIRBuilder : public MachineIRBuilder {
/// dominates the current insertion point and if not, move it just before the
/// current insertion point and return it. If not found, return Null
/// MachineInstrBuilder.
- MachineInstrBuilder getDominatingInstrForID(FoldingSetNodeID &ID,
- void *&NodeInsertPos);
+ MachineInstrBuilder
+ getDominatingInstrForID(FoldingSetNodeID &ID, void *&NodeInsertPos,
+ SmallVectorImpl<DILocation *> *Locs = nullptr);
/// Simple check if we can CSE (we have the CSEInfo) or if this Opcode is
/// safe to CSE.
bool canPerformCSEForOpc(unsigned Opc) const;
@@ -97,8 +98,9 @@ class CSEMIRBuilder : public MachineIRBuilder {
// Bring in the other overload from the base class.
using MachineIRBuilder::buildConstant;
- MachineInstrBuilder buildConstant(const DstOp &Res,
- const ConstantInt &Val) override;
+ MachineInstrBuilder
+ buildConstant(const DstOp &Res, const ConstantInt &Val,
+ SmallVectorImpl<DILocation *> *Locs = nullptr) override;
// Bring in the other overload from the base class.
using MachineIRBuilder::buildFConstant;
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h b/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
index 305bef7dd3ea63..266bdb80614f01 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
@@ -99,8 +99,12 @@ class LegalizationArtifactCombiner {
const LLT DstTy = MRI.getType(DstReg);
if (isInstLegal({TargetOpcode::G_CONSTANT, {DstTy}})) {
auto &CstVal = SrcMI->getOperand(1);
+ SmallVector<DILocation *> Locs;
+ Locs.push_back(MI.getDebugLoc().get());
+ Locs.push_back(SrcMI->getDebugLoc().get());
Builder.buildConstant(
- DstReg, CstVal.getCImm()->getValue().sext(DstTy.getSizeInBits()));
+ DstReg, CstVal.getCImm()->getValue().sext(DstTy.getSizeInBits()),
+ &Locs);
UpdatedDefs.push_back(DstReg);
markInstAndDefDead(MI, *SrcMI, DeadInsts);
return true;
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
index e15f7a7172e1a4..dc67a3321a0d9d 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
@@ -856,8 +856,9 @@ class MachineIRBuilder {
/// type.
///
/// \return The newly created instruction.
- virtual MachineInstrBuilder buildConstant(const DstOp &Res,
- const ConstantInt &Val);
+ virtual MachineInstrBuilder
+ buildConstant(const DstOp &Res, const ConstantInt &Val,
+ SmallVectorImpl<DILocation *> *Locs = nullptr);
/// Build and insert \p Res = G_CONSTANT \p Val
///
@@ -868,7 +869,9 @@ class MachineIRBuilder {
///
/// \return The newly created instruction.
MachineInstrBuilder buildConstant(const DstOp &Res, int64_t Val);
- MachineInstrBuilder buildConstant(const DstOp &Res, const APInt &Val);
+ MachineInstrBuilder
+ buildConstant(const DstOp &Res, const APInt &Val,
+ SmallVectorImpl<DILocation *> *Locs = nullptr);
/// Build and insert \p Res = G_FCONSTANT \p Val
///
diff --git a/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp b/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
index 551ba1e6036c17..893bebf4da327d 100644
--- a/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp
@@ -36,7 +36,8 @@ bool CSEMIRBuilder::dominates(MachineBasicBlock::const_iterator A,
MachineInstrBuilder
CSEMIRBuilder::getDominatingInstrForID(FoldingSetNodeID &ID,
- void *&NodeInsertPos) {
+ void *&NodeInsertPos,
+ SmallVectorImpl<DILocation *> *Locs) {
GISelCSEInfo *CSEInfo = getCSEInfo();
assert(CSEInfo && "Can't get here without setting CSEInfo");
MachineBasicBlock *CurMBB = &getMBB();
@@ -51,6 +52,11 @@ CSEMIRBuilder::getDominatingInstrForID(FoldingSetNodeID &ID,
// this builder will have the def ready.
setInsertPt(*CurMBB, std::next(MII));
} else if (!dominates(MI, CurrPos)) {
+ if (Locs) {
+ Locs->push_back(MI->getDebugLoc().get());
+ auto *Loc = DILocation::getMergedLocations(*Locs);
+ MI->setDebugLoc(Loc);
+ }
CurMBB->splice(CurrPos, CurMBB, MI);
}
return MachineInstrBuilder(getMF(), MI);
@@ -320,8 +326,9 @@ MachineInstrBuilder CSEMIRBuilder::buildInstr(unsigned Opc,
return memoizeMI(NewMIB, InsertPos);
}
-MachineInstrBuilder CSEMIRBuilder::buildConstant(const DstOp &Res,
- const ConstantInt &Val) {
+MachineInstrBuilder
+CSEMIRBuilder::buildConstant(const DstOp &Res, const ConstantInt &Val,
+ SmallVectorImpl<DILocation *> *Locs) {
constexpr unsigned Opc = TargetOpcode::G_CONSTANT;
if (!canPerformCSEForOpc(Opc))
return MachineIRBuilder::buildConstant(Res, Val);
@@ -337,7 +344,7 @@ MachineInstrBuilder CSEMIRBuilder::buildConstant(const DstOp &Res,
profileMBBOpcode(ProfBuilder, Opc);
profileDstOp(Res, ProfBuilder);
ProfBuilder.addNodeIDMachineOperand(MachineOperand::CreateCImm(&Val));
- MachineInstrBuilder MIB = getDominatingInstrForID(ID, InsertPos);
+ MachineInstrBuilder MIB = getDominatingInstrForID(ID, InsertPos, Locs);
if (MIB) {
// Handle generating copies here.
return generateCopiesIfRequired({Res}, MIB);
diff --git a/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp b/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
index 2e8407813ba641..5cbdfe6b531dff 100644
--- a/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
@@ -314,8 +314,9 @@ MachineInstrBuilder MachineIRBuilder::buildCopy(const DstOp &Res,
return buildInstr(TargetOpcode::COPY, Res, Op);
}
-MachineInstrBuilder MachineIRBuilder::buildConstant(const DstOp &Res,
- const ConstantInt &Val) {
+MachineInstrBuilder
+MachineIRBuilder::buildConstant(const DstOp &Res, const ConstantInt &Val,
+ SmallVectorImpl<DILocation *> *Locs) {
LLT Ty = Res.getLLTTy(*getMRI());
LLT EltTy = Ty.getScalarType();
assert(EltTy.getScalarSizeInBits() == Val.getBitWidth() &&
@@ -375,10 +376,11 @@ MachineInstrBuilder MachineIRBuilder::buildFConstant(const DstOp &Res,
return Const;
}
-MachineInstrBuilder MachineIRBuilder::buildConstant(const DstOp &Res,
- const APInt &Val) {
+MachineInstrBuilder
+MachineIRBuilder::buildConstant(const DstOp &Res, const APInt &Val,
+ SmallVectorImpl<DILocation *> *Locs) {
ConstantInt *CI = ConstantInt::get(getMF().getFunction().getContext(), Val);
- return buildConstant(Res, *CI);
+ return buildConstant(Res, *CI, Locs);
}
MachineInstrBuilder MachineIRBuilder::buildFConstant(const DstOp &Res,
diff --git a/llvm/test/DebugInfo/merge-locations-legalizer.mir b/llvm/test/DebugInfo/merge-locations-legalizer.mir
new file mode 100644
index 00000000000000..92bd5de2858e69
--- /dev/null
+++ b/llvm/test/DebugInfo/merge-locations-legalizer.mir
@@ -0,0 +1,58 @@
+# RUN: llc %s -O0 --start-before=legalizer --stop-after=legalizer -o - | FileCheck %s
+# CHECK-NOT: %35:_(s32) = G_CONSTANT i32 0, debug-location !71
+# CHECK: %35:_(s32) = G_CONSTANT i32 0, debug-location !DILocation(line: 0,
+--- |
+ define i32 @main(i32 %0, ptr %1) #0 !dbg !57 {
+ entry:
+ ret i32 0, !dbg !71
+ }
+ !3 = !DIFile(filename: "/Volumes/Data/swift/llvm-project/lldb/test/API/lang/swift/external_provider_extra_inhabitants/main.swift", directory: "/Volumes/Data/swift")
+ !23 = distinct !DICompileUnit(language: DW_LANG_Swift, file: !3, sdk: "MacOSX14.2.sdk")
+ !57 = distinct !DISubprogram(name: "main", unit: !23)
+ !64 = distinct !DILexicalBlock(scope: !57, column: 1)
+ !66 = distinct !DILexicalBlock(scope: !64, column: 1)
+ !68 = !DILocation(line: 12, scope: !66)
+ !70 = distinct !DILexicalBlock(scope: !66, column: 1)
+ !71 = !DILocation(line: 13, scope: !70)
+name: main
+registers:
+ - { id: 0, class: _, preferred-register: '' }
+ - { id: 1, class: _, preferred-register: '' }
+ - { id: 2, class: _, preferred-register: '' }
+ - { id: 3, class: _, preferred-register: '' }
+ - { id: 4, class: _, preferred-register: '' }
+ - { id: 5, class: _, preferred-register: '' }
+ - { id: 6, class: _, preferred-register: '' }
+ - { id: 7, class: _, preferred-register: '' }
+ - { id: 8, class: _, preferred-register: '' }
+ - { id: 9, class: _, preferred-register: '' }
+ - { id: 10, class: _, preferred-register: '' }
+ - { id: 11, class: _, preferred-register: '' }
+ - { id: 12, class: _, preferred-register: '' }
+ - { id: 13, class: _, preferred-register: '' }
+ - { id: 14, class: _, preferred-register: '' }
+ - { id: 15, class: gpr64, preferred-register: '' }
+ - { id: 18, class: _, preferred-register: '' }
+ - { id: 19, class: _, preferred-register: '' }
+ - { id: 20, class: _, preferred-register: '' }
+ - { id: 21, class: _, preferred-register: '' }
+ - { id: 22, class: _, preferred-register: '' }
+ - { id: 23, class: _, preferred-register: '' }
+ - { id: 24, class: _, preferred-register: '' }
+ - { id: 25, class: _, preferred-register: '' }
+ - { id: 26, class: _, preferred-register: '' }
+ - { id: 27, class: _, preferred-register: '' }
+ - { id: 28, class: _, preferred-register: '' }
+ - { id: 29, class: _, preferred-register: '' }
+ - { id: 30, class: _, preferred-register: '' }
+ - { id: 31, class: _, preferred-register: '' }
+ - { id: 32, class: _, preferred-register: '' }
+ - { id: 33, class: _, preferred-register: '' }
+ - { id: 34, class: _, preferred-register: '' }
+body: |
+ bb.1.entry:
+ %16:_(s8) = G_CONSTANT i8 0, debug-location !68
+ %17:_(s32) = G_ANYEXT %16(s8), debug-location !68
+ $w2 = COPY %17(s32), debug-location !68
+ %35:_(s32) = G_CONSTANT i32 0, debug-location !71
+ $w0 = COPY %35(s32), debug-location !71
|
rastogishubham
force-pushed
the
FixLegalizer
branch
from
6b01b3d to
66e78da
Compare
rastogishubham
force-pushed
the
FixLegalizer
branch
3 times, most recently
from
74ca6ac to
6a9a383
Compare
arsenm
reviewed
May 3, 2024
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
Outdated
Show resolved
Hide resolved
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
Outdated
Show resolved
Hide resolved
rastogishubham
force-pushed
the
FixLegalizer
branch
2 times, most recently
from
e6fe227 to
2d4fc05
Compare
| @@ -99,6 +100,8 @@ class LegalizationArtifactCombiner { | |||
| const LLT DstTy = MRI.getType(DstReg); | |||
| if (isInstLegal({TargetOpcode::G_CONSTANT, {DstTy}})) { | |||
| auto &CstVal = SrcMI->getOperand(1); | |||
| Builder.MergedLocation = DILocation::getMergedLocation( | |||
There was a problem hiding this comment.
I don't think I like this solution of keeping global state in the MachineBuilder over threading it through the API.
There was a problem hiding this comment.
@arsenm I concur with Adrian, one of the issues with the current approach is that there are multiple code paths that reach CSEMIRBuilder::getDominatingInstrForID so I have to clear the MergedLocation right after using it for the dominating instruction. If pass the DILocation through the API, we can just have a default nullptr value on it, and it doesn't change much.
arsenm
reviewed
May 7, 2024
| @@ -243,6 +243,10 @@ class MachineIRBuilder { | |||
| } | |||
|
|
|||
| public: | |||
| /// This is the merged location of an instruction which is a result of a fold | |||
| /// in the legalizer. | |||
| DILocation *MergedLocation = nullptr; | |||
There was a problem hiding this comment.
This doesn't seem like block or function level state
There was a problem hiding this comment.
I don't know what that means
There was a problem hiding this comment.
It means it doesn't belong in MachineIRBuilder
rastogishubham
force-pushed
the
FixLegalizer
branch
from
2d4fc05 to
7d19155
Compare
rastogishubham
force-pushed
the
FixLegalizer
branch
from
7d19155 to
c12c682
Compare
arsenm
requested changes
May 7, 2024
rastogishubham
force-pushed
the
FixLegalizer
branch
from
c12c682 to
3fc9ce4
Compare
|
@arsenm I addressed your feedback, does the patch look okay? |
|
@adrian-prantl @arsenm @aemerson ping! |
adrian-prantl
approved these changes
May 11, 2024
arsenm
approved these changes
May 13, 2024
| # (%1, and %2 in the test) in the legalizer, the DILocation of the | ||
| # instruction that is moved (%3) is updated appropriately. | ||
|
|
||
| #REQUIRES: aarch64-registered-target |
There was a problem hiding this comment.
I would assume this is implied by the AArch64 directory
arsenm
reviewed
May 13, 2024
| # (%1, and %2 in the test) in the legalizer, the DILocation of the | ||
| # instruction that is moved (%3) is updated appropriately. | ||
|
|
||
| #REQUIRES: aarch64-registered-target |
There was a problem hiding this comment.
Suggested change
| #REQUIRES: aarch64-registered-target |
rastogishubham
force-pushed
the
FixLegalizer
branch
from
3fc9ce4 to
4ff51e7
Compare
Make sure to merge the sourcelocation of the Dominating Instruction that is hoisted in a basic block in the CSEMIRBuilder in the legalizer pass. If this is not done, we can have a incorrect line table entry that makes the instruction pointer jump around. For example the line table without this patch looks like: Address Line Column File ISA Discriminator OpIndex Flags ------------------ ------ ------ ------ --- ------------- ------- ------------- 0x0000000000000000 0 0 1 0 0 0 is_stmt 0x0000000000000010 11 14 1 0 0 0 is_stmt prologue_end 0x0000000000000028 12 1 1 0 0 0 is_stmt 0x000000000000002c 12 15 1 0 0 0 0x000000000000004c 12 13 1 0 0 0 0x000000000000005c 13 1 1 0 0 0 is_stmt 0x0000000000000064 12 13 1 0 0 0 is_stmt 0x000000000000007c 13 7 1 0 0 0 is_stmt 0x00000000000000c8 13 1 1 0 0 0 0x00000000000000e8 13 1 1 0 0 0 epilogue_begin 0x00000000000000f8 13 1 1 0 0 0 end_sequence The line table entry for 0x000000000000005c should be 0
rastogishubham
force-pushed
the
FixLegalizer
branch
from
4ff51e7 to
0604619
Compare
rastogishubham
added a commit
to rastogishubham/llvm-project
that referenced
this pull request
May 16, 2024
…#90922) Make sure to merge the sourcelocation of the Dominating Instruction that is hoisted in a basic block in the CSEMIRBuilder in the legalizer pass. If this is not done, we can have a incorrect line table entry that makes the instruction pointer jump around. For example the line table without this patch looks like: ``` Address Line Column File ISA Discriminator OpIndex Flags ------------------ ------ ------ ------ --- ------------- ------- ------------- 0x0000000000000000 0 0 1 0 0 0 is_stmt 0x0000000000000010 11 14 1 0 0 0 is_stmt prologue_end 0x0000000000000028 12 1 1 0 0 0 is_stmt 0x000000000000002c 12 15 1 0 0 0 0x000000000000004c 12 13 1 0 0 0 0x000000000000005c 13 1 1 0 0 0 is_stmt 0x0000000000000064 12 13 1 0 0 0 is_stmt 0x000000000000007c 13 7 1 0 0 0 is_stmt 0x00000000000000c8 13 1 1 0 0 0 0x00000000000000e8 13 1 1 0 0 0 epilogue_begin 0x00000000000000f8 13 1 1 0 0 0 end_sequence ``` The line table entry for 0x000000000000005c should be 0 After this patch, the line table looks like: ``` Address Line Column File ISA Discriminator OpIndex Flags ------------------ ------ ------ ------ --- ------------- ------- ------------- 0x0000000000000000 0 0 1 0 0 0 is_stmt 0x0000000000000010 11 14 1 0 0 0 is_stmt prologue_end 0x0000000000000028 12 1 1 0 0 0 is_stmt 0x000000000000002c 12 15 1 0 0 0 0x000000000000004c 12 13 1 0 0 0 0x000000000000005c 0 0 1 0 0 0 0x0000000000000064 12 13 1 0 0 0 0x000000000000007c 13 7 1 0 0 0 is_stmt 0x00000000000000c8 13 1 1 0 0 0 0x00000000000000e8 13 1 1 0 0 0 epilogue_begin 0x00000000000000f8 13 1 1 0 0 0 end_sequence ```
adrian-prantl
added a commit
to swiftlang/llvm-project
that referenced
this pull request
May 16, 2024
Merge sourcelocation in CSEMIRBuilder::getDominatingInstrForID. (llvm#90922)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make sure to merge the sourcelocation of the Dominating Instruction that is hoisted in a basic block in the CSEMIRBuilder in the legalizer pass.
If this is not done, we can have a incorrect line table entry that makes the instruction pointer jump around.
For example the line table without this patch looks like:
The line table entry for 0x000000000000005c should be 0
After this patch, the line table looks like: