[RISC-V] Add SMLoc info for fixup. [NFCI] #142054
Conversation
This makes sure that potential error messages attached to the fixups being created are linked to the source code that triggered the split.
|
@llvm/pr-subscribers-backend-risc-v Author: Francesco Petrogalli (fpetrogalli) ChangesThis makes sure that potential error messages attached to the fixups being created are linked to the source code that triggered the split. Full diff: https://github.com/llvm/llvm-project/pull/142054.diff 1 Files Affected:
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
index 963501db118e7..7b15a01dcf9ec 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
@@ -645,8 +645,9 @@ bool RISCVAsmBackend::addReloc(const MCFragment &F, const MCFixup &Fixup,
}
MCValue A = MCValue::get(Target.getAddSym(), nullptr, Target.getConstant());
MCValue B = MCValue::get(Target.getSubSym());
- auto FA = MCFixup::create(Fixup.getOffset(), nullptr, TA);
- auto FB = MCFixup::create(Fixup.getOffset(), nullptr, TB);
+ const SMLoc Loc = Fixup.getLoc();
+ auto FA = MCFixup::create(Fixup.getOffset(), nullptr, TA, Loc);
+ auto FB = MCFixup::create(Fixup.getOffset(), nullptr, TB, Loc);
Asm->getWriter().recordRelocation(F, FA, A, FixedValueA);
Asm->getWriter().recordRelocation(F, FB, B, FixedValueB);
FixedValue = FixedValueA - FixedValueB;
|
|
I actually plan to remove SMLoc from MCFixup to reduce its size, as the SMLoc can typically be retrieved from I also considered removing SMLoc from MCExpr. https://llvm-compile-time-tracker.com/compare.php?from=193135c80019a01e08014b5727088b860a09f493&to=b8a413de2798260fabf6b5650fc22c803fa13831&stat=max-rss&linkStats=on The saving is actually quite decent for a -g build, but some folks might need the MCExpr loc. stage1-ReleaseLTO-g (link only):Benchmark | Old | New-- | -- | -- I think we should remove SMLoc from either MCFixup or MCExpr. If we end up updating MCFixup, this change will be rewritten. |
I don't understand how SMLoc is saving size on these non-LLVM executable.
I don't have preferences, as long as we can still prompt errors that point to the line that is causing the error. Some context on why I created this MR. I am currently debugging some failures in the relocations for Macho-O, adding these SMLoc helped me out to figure out what was wrong. I can keep these changes locally if you don't find them useful. Even if we merged this change, I don't have a regression test for it (other than the failure I am debugging, which of course I cannot upstream as it is), so if you end up removing it in the cleanup, nobody will notice. |
This page demonstrates that a stage1 build of LLVM with my commit applied (removing SMLoc from MCExpr), will use less memory when building these llvm-test-suite projects. If we keep MCExpr location, we can remove the MCFixup location or repurose the field for something else (perhaps more similar to GNU Assembler).
|
Thanks for the explanation. Let me summarize:
I don't have anything against removing SMLoc from MCFixup, but I found the changes in this MR useful for my own development process. I think we should get them in: if you end up removing SMLoc from MCFixup, the functionality provided by this change will likely still be there via MCExpr and the link it has with MCFixup. |
In your development process, can you use the SMLoc from I plan to do so and remove MCFixup::Loc soon. MCFixup::Value being nullptr is a very uncommon special case (used by RISCVAsmBackend as a minor optimization). |
This makes sure that potential error messages attached to the fixups being created are linked to the source code that triggered the split.