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

Mon Jun 6 12:10:29 PDT 2022

ABataev 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
----------------
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
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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127119