[PATCH] D157373: [RISCV] add a compress optimization for stack inst.

[Wang Pengcheng via Phabricator via llvm-commits]

wangpc added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:1324
+      // Avoid increasing extra instructions when ld/st can be compressed.
+      if ((CSI.size() <= RVCompressLen) && (StackSize <= RVCompressLen + 2048 ||
+                                            StackSize > 2048 * 3 - StackAlign))
----------------
lcvon007 wrote:
> wangpc wrote:
> > lcvon007 wrote:
> > > wangpc wrote:
> > > > lcvon007 wrote:
> > > > > lcvon007 wrote:
> > > > > > lcvon007 wrote:
> > > > > > > wangpc wrote:
> > > > > > > > Do we really need this condition `(StackSize <= RVCompressLen + 2048 || StackSize > 2048 * 3 - StackAlign)`?
> > > > > > > > Do we really need this condition `(StackSize <= RVCompressLen + 2048 || StackSize > 2048 * 3 - StackAlign)`?
> > > > > > > 
> > > > > > > As you see in the first version of implementation, it will add extra instructions(because the second SP amount may be too larger to use more instructions to build the immediate) if we don't add this condition, and the performance may regress in some cases, and it's better to remove this condition if we only want to optimize the codesize.
> > > > > > > Do we really need this condition `(StackSize <= RVCompressLen + 2048 || StackSize > 2048 * 3 - StackAlign)`?
> > > > > > 
> > > > > > 
> > > > > > Do we really need this condition `(StackSize <= RVCompressLen + 2048 || StackSize > 2048 * 3 - StackAlign)`?
> > > > > 
> > > > > As you see in the first version of implementation, it will add extra instructions(because the second SP amount may be too larger to use more instructions to build the immediate) if we don't add this condition, and the performance may regress in some cases, and it's better to remove this condition if we only want to optimize the codesize.
> > > > If so, then I think we can loose the condition when optimizing for size? What do you think about it?
> > > yes,  is it ok to add this feature in other commit? I actually have other ideas to decrease the code size too so I can try it together.
> > Is `2048 * 3` an empirical value? I'm sorry that I can't figure out the reason.
> > If we want second adjustment to be larger than 2048 , should it be `2048*2-StackAlign`?
> [2048, 2048+512]: before opt: addi + addi , after opt: addi + addi
> (2048 + 512, 2048 * 2 - StackAlign] : before opt: addi + addi, after opt: addi + addi + addi : not good
> (2048 * 2 -stackAlign,  2048 * 2 + 512] before opt: addi + addi + addi, after opt : addi + addi + addi
> (2048 * 2 + 512, 2048 * 3 -StackAlign] : before opt: addi + addi + addi, after opt: addi + lui + addi + addi sp, sp, x?
> (2048 * 3 -StackAlign, +inf): before: addi + lui+ addi + addi +addi sp, sp, x? , after opt: addi + lui+ addi + addi sp, sp, x?
> it's not good to use '2048*2 -StackAlign' but I may add extra condition like 
> if (StackSize <= RVCompressLen + 2048 || (StackSize > 2048 * 2 -stackAlign && StackSize <=2048 * 2 + 512) || StackSize > 2048 * 3 -StackAlign )
OK, I get it. Can we add more tests for different stack sizes as you said? And some suggestions on test: 1) MIRs in test can be further reduced (for example , the call to `memset` is really needed?), 2) rename functions to `stack_size_n` where `n` is the stack size.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157373