[PATCH] D79141: [RISCV] Better Split Stack Pointer Adjustment for RVC

[Alex Bradbury via Phabricator via llvm-commits]

asb requested changes to this revision.
asb added a comment.
This revision now requires changes to proceed.

Thanks Sam. I left a few minor comments inline. From looking at the diff in compiler output on the GCC torture suite, I do see a few cases where there is a potential regression. e.g. loop-ivopts-2.c, which represents a case where the stack adjustment is large, but doesn't overflow the 12-bit immediate fields of addi and s[d|w]. The prologue and epilogue are both two instructions and become three instructions with this patch (although it is a code size win).

I don't expect this is a real world problem and of course there are many microarchitectures where 3 compressed instructions could be preferred. But if we could gate examples in the same class as above on whether we're optimising for size I think that would be slightly preferable.

I haven't looked in more detail but I also see pr37573.c at O1 <https://reviews.llvm.org/owners/package/1/> adds another two instructions to the prologue and epilogue. In the case the code size savings are more meaningful, so there's a strong argument that defaulting to slightly more instructions is a good choice in the common case.

I think this isn't totally clear cut so I'm not expressing a fixed and unchangeable position here, but did want to discuss further before merging.



================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:630
 
-// We would like to split the SP adjustment to reduce prologue/epilogue
-// as following instructions. In this way, the offset of the callee saved
-// register could fit in a single store.
-//   add     sp,sp,-2032
-//   sw      ra,2028(sp)
-//   sw      s0,2024(sp)
-//   sw      s1,2020(sp)
-//   sw      s3,2012(sp)
-//   sw      s4,2008(sp)
-//   add     sp,sp,-64
-uint64_t
-RISCVFrameLowering::getFirstSPAdjustAmount(const MachineFunction &MF) const {
+/**
+ * We might like to split the SP adjustment to reduce prologue/epilogue as shown
----------------
Should be a C++ style comment


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:631
+/**
+ * We might like to split the SP adjustment to reduce prologue/epilogue as shown
+ * in the following instructions. The reason to do this is twofold: so that the
----------------
I'm struggling to parse the "reduce prologue/epilogue" bit of this sentence


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:634
+ * offset for the callee-saved register saves/restores fits into a single
+ * function; and to help the SP adjustment amounts fit into two `addi`
+ * instructions rather than needing to materialise those immediates with extra
----------------
Did you mean to say "function" here?


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.h:60
 
-  // Get the first stack adjustment amount for SplitSPAdjust.
-  // Return 0 if we don't want to to split the SP adjustment in prologue and
-  // epilogue.
-  uint64_t getFirstSPAdjustAmount(const MachineFunction &MF) const;
+  /**
+   * Returns if we want to adjust the SP in two adjustments in the prolog and
----------------
Should use a C++ style comment


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.h:61
+  /**
+   * Returns if we want to adjust the SP in two adjustments in the prolog and
+   * epilog, or keep it as just one.
----------------
Nit: LLVM seems to mostly use prologue and epilogue spellings


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.h:65
+   * The result will be `None` if the SP adjustment should not be split, or an
+   * optional containing the first SP adjustment amount if it should be split.
+   */
----------------
Nit: optional => Opetional


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79141