[PATCH] D158623: [RISCV] Reorder the stack frame objects.

[Wang Pengcheng via Phabricator via llvm-commits]

wangpc added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:476
+
+struct RISCVFrameSortingComparator {
+  inline bool operator()(const RISCVFrameSortingObject &A,
----------------
Just a style taste. Can we replace this struct with lambda?


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:499
+    // arithmetic.
+    DensityAScaled = static_cast<uint64_t>(A.ObjectNumUses) *
+                     static_cast<uint64_t>(B.ObjectSize);
----------------
We don't need `static_cast` I think.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:545
+  // It's only used to reduce codesize.
+  if (!MF.getFunction().hasMinSize())
+    return;
----------------
`hasMinSize()` means that we only enable this optimization in `-Oz`, not in `-Os`. Is this expected?
`hasOptSize()` is for both `-Os` and `-Oz`.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:611
+  // Flip it if we're accessing off of the FP.
+  if (!RI->hasStackRealignment(MF) && hasFP(MF))
+    std::reverse(ObjectsToAllocate.begin(), ObjectsToAllocate.end());
----------------
This should be tested. Please add a MIR test that uses FP.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:613
+    std::reverse(ObjectsToAllocate.begin(), ObjectsToAllocate.end());
+}
+
----------------
Please add a `LLVM_DEBUG` to print the final frame just like AArch64.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158623