[PATCH] D140493: [SROA] Support promotion in presence of variably-indexed loads

Thu Dec 22 17:55:51 PST 2022

lebedev.ri added a comment.

(added x86 codegen tests in e7f21d750cc51f5a9610b7f13586b2b6907c6097 <https://reviews.llvm.org/rGe7f21d750cc51f5a9610b7f13586b2b6907c6097>)

In D140493#4014396 <https://reviews.llvm.org/D140493#4014396>, @craig.topper wrote:

>> @craig.topper i'm somewhat confused with that RISC-V codegen.
>> The fact that it scalarizes *all* vector loads is a pretty glaring bug,
>> which a bit disqualifies cost modelling question, since there are
>> wider integer loads, that could be used instead. I'm rather not familiar
>> with that target, so i'm not sure if i should look into it.
>
> I haven't looked but I assume that's the type legalizer just doing SplitVector repeatedly and then doing ScalarizeVector. There is no intelligence for loads that are only used by stores.

Yes, i have looked before posting that comment, and yes that is what happens.

> NOTE: RISC-V also requires all scalar loads to be naturally aligned which can introduce additional restrictions on how a vector load/store can be scalarized.

Aha, i had forgotten about that. That does complicate things, to say the least,
but then the pre-SROA codegen would be just as, err, uninspiring:
this is the current codegen diff https://godbolt.org/z/34WEGfMjj

Would you say that as far as RISCV is concerned guarding with a simple profitability check
of "alloca size must not be more than 2x the largest legal integer size
(and we expect we have good variable shifts for largest legal integer size)"
is not sufficient and a TTI hook is needed?

>> And after that, we are back to the question:
>>
>> is there support for shifts in base ISA?
>> what is the widest integer type that can be shifted?
>
> Yes there are shifts. riscv32 has 32-bit shifts. riscv64 has 64-bit shifts. smaller shifts need to promoted. wider shifts will be expanded. There is no cmov so expansion requires a select to be expanded to control flow.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140493