Details
Diff Detail
Event Timeline
int global_a = 10;
__attribute__((noinline)) int test2() {
int a = global_a ;
int b = 10;
return a + b;
}- Command： clang --target=riscv32 -g -S x.c -o x.s
- After the patch is merged, the prologue will be placed in the correct position
Is removing the if (I != MBB.end()) DL = I->getDebugLoc(); actually correct? I'm wondering why do several other targets have that if it can just be removed.
How about instead adding a MachineInstr::FrameSetup flag to the appropriate instructions? That also seems to fix the issue. Don't we want to add that flag anyway?
Does the test actually need to check the DWARF? Can't we just check the assembly, namely that the .loc prologue_end directive is inserted in the correct place?
I don't understand how this is a problem for the current test, https://godbolt.org/z/YohW4nn87 does not produce the assembly you claim it does
| llvm/test/DebugInfo/RISCV/dbg-prolog-end.ll | ||
|---|---|---|
| 4–5 ↗ | (On Diff #440941) | Redundant (and get in the way of testing RV64) |
| 7–19 ↗ | (On Diff #440941) | The IR is simple, C isn't needed |
Ok, with -frame-pointer=all in the command it produces the incorrect directive, so any test *must* set that for the function attributes otherwise it's not testing anything
Isn't the problem just that spill/restoreCalleeSavedRegisters need to set FrameSetup/Destroy on their stores/loads (which is also a problem with the generic implementation that we used to use before implementing -msave-restore)?
Annoyingly storeRegToStackSlot/loadRegFromStackSlot don't take a Flags argument to pass it through. X86 hacks around this with:
--MI; MI->setFlag(MachineInstr::FrameSetup); ++MI;
after the call to storeRegToStackSlot (and doesn't bother for FrameDestroy, I guess that matters less, or people care about it less when debugging). I believe we can do the same hack, but we should probably fix it properly by adding a Flags argument to them, and update the generic implementation too.
Normally these dwarf information is needed when the debugger is debugging. I don't think it is necessary to add debugging information at this stage, so i think we need remove unused debug-location instead of
adding FrameSetup at this stage（spill/restoreCalleeSavedRegisters）.
As shown above, so I removed the debugLoc information at storeRegToStackSlot， also I think also need removed the debugloc at loadRegFromSatckSlot.
In fact, the key problem is the condition of prologue_end generation， please see DwarfDebug::beginInstuction
> Does the test actually need to check the DWARF? Can't we just check the assembly, namely that the .loc prologue_end directive is inserted in the correct place?
Need Dwarf
Through Dwarf, I am verifying that the address of the current Prologue_end is the address of the end of the fuction prologue
I think we may remove debug-location instead of adding FrameSetup flags for spill/restoreCalleeSavedRegisters, Because dwarf information is added for debugging convenience， At this stage, people should not care when debugging
I think removing debug location is problematic. Because storeRegToStackSlot method is used in many places, for example, register spillers use it to spill a register to stack frame. If the spill instruction's debug location info were removed, its debug location would follow the previous instruction's location when emitting debug location info. It is ok to do that in -O0, but if the optimization level were -O2, instructions would be hoisted or sink which would cause the spill instruction to have wrong debug location info.
FYI I looked before at adding a flags arg to storeRegToStackSlot, but ultimately got distracted with other things https://reviews.llvm.org/D30115
I think the user should not care about the debugging information in this scenario. So I removed the debug-location. I am curious, under what circumstances would users care about the debug information of the register spill instruction? What may be the problem, you need to locate the debug information of the register spill instruction?
I was wrong. According to https://reviews.llvm.org/D52125, instructions artificially generated by the compiler should not attach any debug locations.
The test should be placed in test/CodeGen/RISCV, instead of in test/DebugInfo/RISCV. It is more about the .loc directive which does not fit so well into the purpose of test/DebugInfo .
Having separate rv32/rv64 seems excessive to me, too. The tests have too much noise which can use some cleanup.
Just check .loc in the llc output. Avoid llvm-dwarfdump --debug-line/llvm-objdump.
Just check .loc in the llc output. Avoid llvm-dwarfdump --debug-line/llvm-objdump.
The main purpose of using llvm-objdump and llvm-dwarfdump is to determine the position where .loc prologue_end is inserted into the end of the function prologue
Looks like there is a relevant discussion: https://github.com/llvm/llvm-project/issues/56370
- I think that spills/reload instructions are compiler-generated without relation to the source-code being compiled and would be better off without debug locations. So we neednt get DL Information in loadRegFromStackSlot/storeRegToStackSlot
- The insertion of wrong debug information leads to the wrong insertion of prologue_end, as shown in the following figure
