[llvm] [LSR] Recognize vscale-relative immediates (PR #88124)

Graham Hunter via llvm-commits llvm-commits at lists.llvm.org
Fri Jun 14 08:43:59 PDT 2024


huntergr-arm wrote:

> I'm not going to bang the "we should be using StackOffset everywhere" drum this time but there are certainly places within this patch where it would make things safer.
> 
> The hardest part of reviewing the patch is all the `getFixedValue()` calls where it's not clear why it safe to assume a fixed offset. There are part of the code where I think we should be able to use the operator overloading better to make the code agnostic to being fixed/scalable. Even for parts that are specific to one of those I think the code would be cleaner if it didn't look special.
> 
> Do you think it's possible to incorporate a scalable safe type without necessarily adding the scalable safe code paths? I'm not asking for a separate PR but the genuine improvements that would be easier to spot if they were in a separate commit separate from the refactoring.

I'm not strictly opposed to using StackOffset, and it may simplify some of the other parts of the code. I used a mutually-exclusive struct here because there are places where it assumes it can make a combined offset between the current known min and max that must be legal, and asserts as much. If we cannot establish ordering between scalable and fixed offsets, then the whole min/max thing breaks.

So we could maybe use StackOffset, but basically use them in the same way Immediate is now, and track scalable and fixed min/max separately.

https://github.com/llvm/llvm-project/pull/88124


More information about the llvm-commits mailing list