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

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg 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) {}
----------------
Can we rename this to something like `CSRSavedLocation`?


================
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;
----------------
`0`/`$noreg` should be considered an invalid register here, no?


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


================
Comment at: llvm/lib/CodeGen/CFIInstrInserter.cpp:216
+        break;
+      case MCCFIInstruction::OpRegister:
+        CSRReg = CFI.getRegister2();
----------------
Can we also have tests for `cfi_register` and `cfi_rel_offset`? My understanding is that we currently don't generate 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() &&
----------------
`it` is not used here


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


================
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
----------------
Can we add the `CFI_INSTRUCTION restore`s for consistency here too?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D74303