[PATCH] D143463: [X86] Use the CFA when appropriate for better variable locations around calls.

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

Thank you for the patch! Sorry for the long delay in review

As a bit of a side-note, I think X86 always maintains a "precise CFA" to refer to, but that may not always be true (see https://reviews.llvm.org/D14948).

I'm curious about a couple points:

- If we do always have the CFA available, then is there any reason not to use it unconditionally?
- If we cannot use it unconditionally, can the condition be more direct, i.e. can we remove the heuristic?
- Is the "frame base" value ABI-stable? If not, is there a strong reason not to rework the variable expressions to instead be CFA-relative, and then just define the "frame base" to always be equal to the CFA? It would avoid the extra offset needed in the current version of the patch.



================
Comment at: llvm/lib/CodeGen/CFIInstrInserter.cpp:153
       MF.getSubtarget().getFrameLowering()->getInitialCFARegister(MF);
-  const TargetRegisterInfo &TRI = *MF.getSubtarget().getRegisterInfo();
+  InitialRegister = TRI.getDwarfRegNum(InitialRegister, true);
   unsigned NumRegs = TRI.getNumRegs();
----------------
Fairly harmless (and NFC in this case) but I don't believe `Register` is intended to be constructed with a dwarf register ordinal


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:3642
 
 Register
 X86FrameLowering::getInitialCFARegister(const MachineFunction &MF) const {
----------------
wrt. the type confusion here, it seems to be a bug introduced during a bulk update which replaced many instances of `unsigned` with `Register` (2481f26ac3f22)


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:3651-3652
+  Register FrameRegister = RI->getFrameRegister(MF);
+  if (getInitialCFARegister(MF) == FrameRegister &&
+      MF.getInfo<X86MachineFunctionInfo>()->hasCFIAdjustCfa()) {
+    DwarfFrameBase FrameBase;
----------------
scott.linder wrote:
> 
Can the new `X86MachineFunctionInfo` field be eliminated, and this instead check the same condition the rest of the X86 code checks before creating a `OpAdjustCfaOffset`? It seems to consistently just be that `!X86FrameLowering::hasFP(MF)`.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:3651-3658
+  if (getInitialCFARegister(MF) == FrameRegister &&
+      MF.getInfo<X86MachineFunctionInfo>()->hasCFIAdjustCfa()) {
+    DwarfFrameBase FrameBase;
+    FrameBase.Kind = DwarfFrameBase::CFA;
+    FrameBase.Location.Offset =
+        -MF.getFrameInfo().getStackSize() - getInitialCFAOffset(MF);
+    return FrameBase;
----------------



================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:2603
       OutStreamer->emitCFIAdjustCfaOffset(-stackGrowth);
+      MF->getInfo<X86MachineFunctionInfo>()->setHasCFIAdjustCfa(true);
     }
----------------
I think the fact that we need this extra call is evidence that the heuristic-based approach and a field in `X86MachineFunctionInfo` is not ideal


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143463/new/

https://reviews.llvm.org/D143463