Page MenuHomePhabricator

[RISCV] Fix wrong position of prologue_end
Needs ReviewPublic

Authored by Miss_Grape on Jun 29 2022, 5:08 AM.

Diff Detail

Event Timeline

Miss_Grape created this revision.Jun 29 2022, 5:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2022, 5:08 AM
Miss_Grape requested review of this revision.Jun 29 2022, 5:08 AM
Miss_Grape retitled this revision from [RISCV] Remove DL Information when storeRegToStackSlot to [RISCV] Fix wrong position progue_end.Jun 29 2022, 5:09 AM
Miss_Grape added a comment.EditedJun 29 2022, 5:28 AM
int global_a = 10;
__attribute__((noinline)) int test2() {
  int a = global_a ;
  int b = 10;
  return a + b;
}
  1. Command: clang --target=riscv32 -g -S x.c -o x.s
  2. After the patch is merged, the prologue will be placed in the correct position

Miss_Grape retitled this revision from [RISCV] Fix wrong position progue_end to [RISCV] Fix wrong position of prologue_end.Jun 29 2022, 5:31 AM

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.

Miss_Grape added a comment.EditedJun 29 2022, 7:02 PM

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?

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.

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.

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

address the comments

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

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

This comment was removed by Miss_Grape.

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.

asb added a comment.Jun 30 2022, 7:56 AM

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

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?

FYI I looked before at adding a flags arg to storeRegToStackSlot, but ultimately got distracted with other things https://reviews.llvm.org/D30115

Thanks,I'll continue to see what to do with the storeRegToStackSlot

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.

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.

Miss_Grape updated this revision to Diff 442766.Jul 6 2022, 8:38 PM
MaskRay added a comment.EditedJul 14 2022, 11:44 AM

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

  1. 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
  1. The insertion of wrong debug information leads to the wrong insertion of prologue_end, as shown in the following figure