[PATCH] D43752: [RISCV] Preserve stack space for outgoing arguments when the function contain variable size objects

Alex Bradbury via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 14 09:23:24 PDT 2018


asb added a comment.

Thanks for making those changes Shiva . The fact we can simplify determineFrameLayout is a good observation.

I've added a few comments, which I think are all comment or formatting related. With those resolved, this looks good to me.

I certainly can't see any unexpected codegen changes when compiling the GCC torture suite with this. Although I'm happy with this and happy for you to commit once resolving the formatting/comment suggestions: if anyone else following this has time to give a second look I'd appreciate it. This is a fiddly part of the codebase, so the more people checking the better.



================
Comment at: lib/Target/RISCV/RISCVFrameLowering.cpp:235-239
+/// hasReservedCallFrame - Under normal circumstances, when a frame pointer is
+/// not required, we reserve argument space for call sites in the function
+/// immediately on entry to the current function.  This eliminates the need for
+/// add/sub sp brackets around call sites.  Returns true if the call frame is
+/// included as part of the stack frame.
----------------
I don't think copy+pasting the description from TargetFrameLowering.h adds much. It would be better to explain what is different about the RISC-V implementation (which is presumably that fact that there is no reserved call frame when there are varsized objects).

I know this is copied from elsewhere in LLVM, but also note that current guidance for Doxygen comments in LLVM is _not_ to repeat the function name at the beginning of the comment https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments


================
Comment at: lib/Target/RISCV/RISCVFrameLowering.cpp:240-241
+/// included as part of the stack frame.
+bool
+RISCVFrameLowering::hasReservedCallFrame(const MachineFunction &MF) const {
+  return !MF.getFrameInfo().hasVarSizedObjects();
----------------
Nit: clang-format is happy for this to be on one line


================
Comment at: lib/Target/RISCV/RISCVFrameLowering.cpp:245
+
+// Eliminate ADJCALLSTACKDOWN, ADJCALLSTACKUP pseudo instructions
+MachineBasicBlock::iterator RISCVFrameLowering::
----------------
Nit: fill stop after comment


================
Comment at: lib/Target/RISCV/RISCVFrameLowering.cpp:246-248
+MachineBasicBlock::iterator RISCVFrameLowering::
+eliminateCallFramePseudoInstr(MachineFunction &MF, MachineBasicBlock &MBB,
+                              MachineBasicBlock::iterator MI) const {
----------------
Nit: clang-format suggests the following:


```
MachineBasicBlock::iterator RISCVFrameLowering::eliminateCallFramePseudoInstr(
    MachineFunction &MF, MachineBasicBlock &MBB,
    MachineBasicBlock::iterator MI) const {

```


================
Comment at: lib/Target/RISCV/RISCVFrameLowering.cpp:253-257
+    // If we have alloca, convert as follows:
+    // ADJCALLSTACKDOWN -> sub, sp, sp, amount
+    // ADJCALLSTACKUP   -> add, sp, sp, amount
+    //
+    // To avoid alloca area poison by outgoing arguments.
----------------
I wonder if this comment could be improved to better explain _why_ this logic is doing what it does. It's certainly true that this helps avoid poisoning of an alloca area due to outgoing arguments, but it's perhaps difficult for someone reading this code to see why. Does the following seem accurate to you?

"""
If space has not been reserved for a call frame, ADJCALLSTACKDOWN and ADJCALLSTACKUP must be converted to instructions manipulating the stack pointer. This is necessary when there is a variable length stack allocation (e.g. alloca), which means it's not possible to allocate space for outgoing arguments from within the function prologue.
"""


================
Comment at: lib/Target/RISCV/RISCVFrameLowering.cpp:261-263
+      // We need to keep the stack aligned properly.  To do this, we round the
+      // amount of space needed for the outgoing arguments up to the next
+      // alignment boundary.
----------------
Nit: If it were me, I'd probably just write "Ensure the stack remains aligned after adjustment.", but opinions vary on what is 'obvious' and the level of detail to put in a comment - so whichever you prefer.


================
Comment at: test/CodeGen/RISCV/alloca.ll:68-70
+  ; Check that outgoing arguments passed on the stack do not corrupt a
+  ; variable-sized stack object.
+  define void @alloca_callframe(i32 %n) nounwind {
----------------
Nit: Should ideally have a new line after the `declare void @func` line, and these lines shouldn't be indented.


Repository:
  rL LLVM

https://reviews.llvm.org/D43752





More information about the llvm-commits mailing list