[PATCH] D99087: [RISCV][WIP] Fix stack slot for argument types (Bug 49500)

Wed Mar 31 06:36:33 PDT 2021

frasercrmck added a comment.

In D99087#2644578 <https://reviews.llvm.org/D99087#2644578>, @luismarques wrote:

> My analysis was a bit rushed. Looking at just the `stack-slot-size.ll` tests, the allocas are fine. The stack has some spare space (due to the correct alignment) and the alloca offset was chosen differently than I had expected when I originally looked at it (36 vs 32), but both options are valid. 
> The other tests I haven't yet looked into in detail to verify if the changes are correct. On the one hand, having this patch with fewer changes is appealing, because it's much easier to review. On the other hand, I see that my patch has reduced the stack size for several test cases, so if those changes are correct they are an improvement, and it would be nice to reap that reward.
> While looking into this issue now, I explored some other patch variants and they had even more test case changes, possibly (correct) improvements, so maybe it's worth it looking into this more carefully? Or maybe, given the priorities, we should just commit this patch for now (looks OK to me)... Decisions, decisions. @asb any thoughts?

Sorry for the slow reply, I've been away from LLVM for a few days, and have kind of been waiting in case @asb had some input. In general, I don't mind which patch goes in but, conceptually, splitting it more into "non-functional" and "optimize stack usage" changes probably sounds like better design. I do think it's worth investigating whether there's work to do on reducing the stack size.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99087