[PATCH] D125787: [RISCV] Fix RVV stack frame alignment bugs

Mon May 23 10:16:40 PDT 2022

reames added a comment.

In D125787#3531852 <https://reviews.llvm.org/D125787#3531852>, @frasercrmck wrote:

> In D125787#3531829 <https://reviews.llvm.org/D125787#3531829>, @reames wrote:
>
>> I prototyped this structure, and all of the tests in tree continued to pass.
>>
>> Given this change has been LGTMed, this is not a blocker, but I think this code organization does make sense as a post cleanup.
>>
>> I am not particular confident in the correctness of this code as it stands.  Either the existing code, or this patch.
>
> Sorry, forgot to reply. The change you propose makes sense to me logically. I can't tell if there are gotchas in reorganizing things like that though.
>
> Since I think discussions have been across various patches at this point, which aspect of this patch are you suspicious of?

A couple of themes, but nothing actionable.

Theme 1 - We have confirmed we have untested code.  We know this code used to differ between two different cases of using SP as the base register.  Its not clear if this is correct, or not.  This implies both lack of test coverage, and (probably) lack of understanding.

Theme 2 - This is very complicated code, and the changes are broad enough to be hard to track.

I am not objecting to this landing.  It's probably more correct than what's in tree.  I'm just saying that even with this, we probably have uncaught issues here.  (Or at least, the code structure doesn't convince me that we *don't*.)  I am a strong proponent of reducing code complexity until it can't be simplified further as a means to flesh out bugs.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125787