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

[Wei Mi via Phabricator via llvm-commits]

wmi marked 7 inline comments as done.
wmi added inline comments.


================
Comment at: llvm/lib/CodeGen/CFIInstrInserter.cpp:92
+  /// contains the location where CSR register is saved.
+  struct CSRRegOffset {
+    CSRRegOffset(unsigned R, int O) : Reg(R), Offset(O) {}
----------------
thegameg wrote:
> Can we rename this to something like `CSRSavedLocation`?
That is better. Done.


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


================
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;
+
----------------
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.


================
Comment at: llvm/lib/CodeGen/CFIInstrInserter.cpp:216
+        break;
+      case MCCFIInstruction::OpRegister:
+        CSRReg = CFI.getRegister2();
----------------
thegameg wrote:
> Can we also have tests for `cfi_register` and `cfi_rel_offset`? My understanding is that we currently don't generate them.
I saw PPC backend has code to generate .cfi_register. I add tests for them. 


================
Comment at: llvm/lib/CodeGen/CFIInstrInserter.cpp:253
+      if (CSRReg != INVALID_REG || CSROffset != INVALID_OFFSET) {
+        auto it = CSRLocMap.find(CFI.getRegister());
+        assert(CSRLocMap.find(CFI.getRegister()) == CSRLocMap.end() &&
----------------
thegameg wrote:
> `it` is not used here
Fixed.


================
Comment at: llvm/lib/CodeGen/CFIInstrInserter.cpp:254
+        auto it = CSRLocMap.find(CFI.getRegister());
+        assert(CSRLocMap.find(CFI.getRegister()) == CSRLocMap.end() &&
+               "CSR location is set repeatedly");
----------------
thegameg wrote:
> Does it mean that we assert if we have:
> 
> ```
> .cfi_offset $rbx, -10
> .cfi_offset $rbx, -24
> ```
> 
> I would think that this is valid and we should just overwrite it. What do you think? If we do want to support it, please add a test as well.
Ah, the assertion is not written in a correct way. The original intention was to be consistent with the assumption above that every function will only save the CSR in one place. If one CSR is saved in multiple different locs, the assertion could catch it.

I change the assertion and add a test for it. 


================
Comment at: llvm/test/CodeGen/X86/cfi-inserter-callee-save-register-2.mir:73
+    $rbp = frame-destroy POP64r implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa $rsp, 8
+    RET 0, killed $rax
----------------
thegameg wrote:
> Can we add the `CFI_INSTRUCTION restore`s for consistency here too?
Done.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D74303