[PATCH] D122594: [ArgPromotion] Update min-legal-vector-width when do promotion

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added a comment.

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

> In D122594#3421352 <https://reviews.llvm.org/D122594#3421352>, @pengfei wrote:
>
>> In D122594#3421178 <https://reviews.llvm.org/D122594#3421178>, @craig.topper wrote:
>>
>>> In D122594#3421073 <https://reviews.llvm.org/D122594#3421073>, @pengfei wrote:
>>>
>>>>> Is this IR illegal because @bar has a min-legal-vector-width of 256 but has 512 bit argument? The inliner can't create broken code from it.
>>>>
>>>> For one thing, if the IR is illegal, shouldn't we fix the place where it turns illegal, i.e., ArgumentPromotion in this example?
>>>> For another, we are inlining `baz` to `foo` rather than `bar`. Do you mean we should scan all other callees of `foo` before the inlining? It doesn't make sense to me.
>>>
>>> I think I wrote "inliner can't create" when I mean to write "inliner can create". I'm fine with fixing the ArgumentPromotion pass if the answer to the question of this IR illegal/unsupported is yes.
>>
>> Oh, I see your point. It's true inliner breaks the code but it's not inliner's problem. As for IR illegal or not, I think it depends on how we explain "min-legal-vector-width".
>>
>>> The min-legal-vector-width was trying to capture where the vectors came from to distinquish vectorizer code from user code.
>>
>> I see the initial purpose of adding "min-legal-vector-width", from that point of view, the IR is legal because it's compiler rather than user creates vector arguments.
>> But the backend implementaion actually takes it as ABI indicator, which determines whether we will use AVX2 calling conversion of AVX512. So I think it is not legal when vector size doesn't match with "min-legal-vector-width".
>>
>>> The vectorizer even with AVX2 frequently generates vectors with more than 256 bits and relies on the backend type legalizer to split them. In order to get similar codegen with prefer-vector-width=256 we had to create the same type legalizer behavior without taking away the vectorizer's freedom to generate oversized vectors.
>>
>> I think there are 2 different cases here:
>>
>> 1. Vectorizer with AVX2 generates 512 bits vector in the function body only;
>> 2. Vectorizer generates 512 bits vector in function arguments;
>>
>> Both will be affected by prefer-vector-width/min-legal-vector-width. But 2) will result in ABI compatibility issue, which is a bug to codegen. ArgPromotion + Inliner is the first one I met with community code, but we have more in our down stream compiler.
>> If middle end pass cares about ABI compatibility, it should handle ABI for the arguments it modifing. That says, when vectorizer with AVX2 generates 512 bits vector, it should turns it into byval rather than put a 512 bits vector in arguments directly. If it creates 256 bits vector with AVX2 or 512 bits vector with AVX512, it should make sure min-legal-vector-width >= the size of the vector argument.
>> When a pass doesn't care ABI compatibility, it is free to create arbitrary vector, but it still needs to make sure min-legal-vector-width >= the size of the vector argument. In case it won't mismatch within the module which might be caused by other optimization, like the inliner problem we met here. Large value of min-legal-vector-width is always safe from the perspective of ABI.
>> Does it make sense to you?
>
> I don’t think the vectorizer touches function arguments.

Yeah, it doesn't matter to be vectorizer. Any pass touches function arguments has the problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122594