[PATCH] D127119: [SLP]Fix undef handling in gather function.

Mon Jun 6 13:27:39 PDT 2022

hvdijk added inline comments.

================
Comment at: llvm/test/Transforms/SLPVectorizer/X86/diamond_broadcast_extra_shuffle.ll:37
 ; CHECK-NEXT:    [[TMP0:%.*]] = insertelement <4 x i32> poison, i32 [[LD]], i32 0
-; CHECK-NEXT:    [[SHUFFLE:%.*]] = shufflevector <4 x i32> [[TMP0]], <4 x i32> poison, <4 x i32> zeroinitializer
-; CHECK-NEXT:    [[TMP1:%.*]] = mul <4 x i32> [[SHUFFLE]], [[SHUFFLE]]
+; CHECK-NEXT:    [[SHUFFLE:%.*]] = shufflevector <4 x i32> [[TMP0]], <4 x i32> poison, <4 x i32> <i32 0, i32 0, i32 0, i32 undef>
+; CHECK-NEXT:    [[SHUFFLE1:%.*]] = shufflevector <4 x i32> [[TMP0]], <4 x i32> poison, <4 x i32> zeroinitializer
----------------
nlopes wrote:
> hvdijk wrote:
> > ABataev wrote:
> > > ABataev wrote:
> > > > hvdijk wrote:
> > > > > ABataev wrote:
> > > > > > nlopes wrote:
> > > > > > > vporpo wrote:
> > > > > > > > hvdijk wrote:
> > > > > > > > > nlopes wrote:
> > > > > > > > > > nikic wrote:
> > > > > > > > > > > hvdijk wrote:
> > > > > > > > > > > > vporpo wrote:
> > > > > > > > > > > > > Doesn't an undef here mean that lane 4 can potentially be poison? Wouldn't that be incorrect?
> > > > > > > > > > > > A mask of undef means the result is undef, not poison, even if the input contains poison elements. See https://llvm.org/docs/LangRef.html#shufflevector-instruction (which is less clear than I would have liked, it used to explicitly say undef, it now says "undefined" to be more readable, but it's not obvious that it is still meant to refer to undef rather than poison)
> > > > > > > > > > > This will be switched to return a poison element in the future, so it would be best not to rely on it. (cc @nlopes).
> > > > > > > > > > Right, thanks!
> > > > > > > > > > We need to switch that behavior to fix a bunch of optimizations.
> > > > > > > > > D93818 linked for reference. Okay, are you saying for the purpose of SLPVectorizer, you would prefer that we already treat undef masks as selecting poison? That should always result in code that is already correct today, but it may not always be optimal under the current rules.
> > > > > > > > Thanks for the explanation!
> > > > > > > That would be awesome, yes!
> > > > > > > It would also motivate us to get that done (@aqjune :)
> > > > > > I'll try to update this patch to follow this new rule
> > > > > I think my D127073 actually already achieved that. The concerns there of regressions there are cases where under the current rules, we can use undef masks, but under those proposed new rules of undef masks, we cannot as we need to ensure the result is not poison. However, this diff is intended to also perform some additional optimisations so it may still be worth updating this one.
> > > > Just one question. Shall I assume that <0, 0, 0, 0> and <0, 0, 0, undef> can be safely merged to <0, 0, 0, 0> then? Or better to wait for D93818?
> > > Yep, my update would be pretty similar to your original patch, just some situation will be handled a bit better. Just need to clarify the question, if shuffle <ld, ld, ld, undef> can be safely replaced by shuffle <ld, ld, ld, ld>
> > If we ensure that the vectorizer only ever generates a mask of <0, 0, 0, undef> when we element 3 is meant to be poison, it will be valid to merge that with <0, 0, 0, 0> even without waiting for D93818, I think: we do not have to support arbitrary existing shufflevectors here that may rely on undef masks giving undef results, we only have to worry about the new shufflevectors we generate.
> > Just need to clarify the question, if shuffle <ld, ld, ld, undef> can be safely replaced by shuffle <ld, ld, ld, ld>
> 
> It's correct to do that in just 2 cases:
>  - ld is known to be non-poison (eg via ValueTracking), or
>  - ld is present in the expression tree already. so x * undef can be replaced with x * x, but not with x * y.
> 
> If instead of undef, you've poison, then it can safely be replaced with ld.
> We've been moving away from initializing vectors with undef, so hopefully undef should be rare in vectors these days. The only exception left is shufflevector.
> It would be nice to duplicate some of the SLP tests to use poison rather than undef to ensure the code is simplifying this appropriately.
This explanation is correct for arbitrary existing shuffles, but I wrote earlier that I do not think that is the case we are concerned with. We are concerned with new shuffle instructions that we generate. If we know that the undef mask was generated by SLPVectorizer (because it is a mask that is not even actually the operand to a shufflevector instruction yet, because it is a mask that we have built up in order to potentially generate a shufflevector instruction), and we decide that in SLPVectorizer, we only generate an undef mask if we originally wanted the result to be poison, that is also a case where we can treat an undef mask as selecting poison.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127119