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

Tue Mar 23 05:58:05 PDT 2021

luismarques added a comment.

> There are certainly fewer changes to existing tests, compared with

D99068 <https://reviews.llvm.org/D99068>. But if @luismarques is right in that there is excessive stack
alignment then this patch needs further work.

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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99087