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

[Wang Pengcheng via Phabricator via llvm-commits]

wangpc accepted this revision.
wangpc added a comment.
This revision is now accepted and ready to land.

LGTM but please let @craig.topper do the final approval.



================
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:
> > > 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`?


================
Comment at: llvm/test/CodeGen/RISCV/stack-inst-compress.mir:13
+  entry:
+    %arr = alloca [517 x i32], align 4
+    call void @llvm.memset.p0.i64(ptr align 4 %arr, i8 0, i64 2068, i1 false)
----------------
Nit: the LLVM IR in a MIR test can be just function stub, the function body can be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157373