[PATCH] D135488: [codegen] Add a remarks based Stack Layout Analysis pass

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added inline comments.


================
Comment at: llvm/lib/CodeGen/StackFrameLayoutAnalysisPass.cpp:44
+
+const char *PassName = "stack-frame-layout";
+
----------------
nickdesaulniers wrote:
> Consider replacing uses of `PassName` with `DEBUG_TYPE` since they have the same value.
ugh, I forgot to update that after adding DEBUG_TYPE. Thanks for pointing that out.


================
Comment at: llvm/lib/CodeGen/StackFrameLayoutAnalysisPass.cpp:106
+    SlotDbgMap SlotMap = genSlotDbgMapping(MF);
+    emitStackFrameLayoutRemarks(MF, SlotMap, Rem);
+
----------------
nickdesaulniers wrote:
> can you sink the call to `genSlotDbgMapping()` into this arg list? `SlotMap` seems unreferenced otherwise.
surprisingly this resulted in a compiler error:

```
StackFrameLayoutAnalysisPass.cpp:104:37: error: non-const lvalue reference to type 'SmallDenseMap<...>' cannot bind to a temporary of type 'SmallDenseMap<...>'
    emitStackFrameLayoutRemarks(MF, genSlotDbgMapping(MF), Rem);
```

So I've just moved it into `emitStackFrameLayoutRemarks()` and dropped the parameter, which avoids the issue entirely. Since SlotMap is only used there now, it makes more sense to structure the code like this anyway.


================
Comment at: llvm/lib/CodeGen/StackFrameLayoutAnalysisPass.cpp:216
+  SlotDbgMap genSlotDbgMapping(MachineFunction &MF) {
+    SlotDbgMap SlotDebugMap;
+    SmallVector<MachineInstr *> Dbg;
----------------
nickdesaulniers wrote:
> If you sink this def into the for loop, then you don't need to clear it.
for some reason I was under the impression that our style guidelines prefered using clear in situations like this, but I can't find it so I think my brain tricked me. Thanks for pointing that out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135488