[PATCH] D74303: [CFI] cfi directive insertion for callee-save-registers in CFIInstrInserter pass

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg accepted this revision.
thegameg added a comment.

Thank you, this LGTM!



================
Comment at: llvm/lib/CodeGen/CFIInstrInserter.cpp:94
+    CSRRegOffset(unsigned R, int O) : Reg(R), Offset(O) {}
+    unsigned Reg = INVALID_REG;
+    int Offset = INVALID_OFFSET;
----------------
wmi wrote:
> thegameg wrote:
> > `0`/`$noreg` should be considered an invalid register here, no?
> Register number used in CFI instruction is got from MCRegisterInfo::getDwarfRegNum, which is different from the physical register number. For $rax, CFI.getRegister() returns 0.   
Maybe `llvm::Optional<T>` is cleaner in this case? (I don't feel strongly about this, though)


================
Comment at: llvm/lib/CodeGen/CFIInstrInserter.cpp:103
+  /// Map the callee save registers to the locations where they are saved.
+  SmallDenseMap<unsigned, CSRRegOffset, 16> CSRLocMap;
+
----------------
wmi wrote:
> thegameg wrote:
> > Isn't this a per-basicblock property? I actually have a hard time coming up with a test case where this would break, so maybe this is sufficient?
> My assumption is every function will only save a CSR in one place and I havn't seen case outside of the assumption. If every basicblock has its own CSR location array, the cost will be much higher, so I choose to hold the assumption until I see a counterexample.
Fair enough. I think it's a safe assumption to make, since for now we only emit the prologue more than once if we have ehfunclet-entry blocks (llvm/test/CodeGen/X86/win64-eh-empty-block-2.mir).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D74303