[PATCH] D57372: Demanded elements support for masked.load and masked.gather

[Philip Reames via Phabricator via llvm-commits]

reames planned changes to this revision.
reames marked an inline comment as done.
reames added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:1455
+            DemandedPtrs.clearBit(i);
+          else if (CElt->isAllOnesValue())
+            DemandedPassThrough.clearBit(i);
----------------
spatel wrote:
> RKSimon wrote:
> > reames wrote:
> > > RKSimon wrote:
> > > > We're not 100% consistent with this but for selects we try to use:
> > > > 
> > > > ```
> > > > if (CElt->isNullValue())
> > > >   DemandedPtrs.clearBit(i);
> > > > else
> > > >   DemandedPassThrough.clearBit(i);
> > > > ```
> > > This would be wrong.  Consider any constant expr or other constant of indeterminate value.  
> > @spatel may be able to advise, but Instruction::Select does this:
> > ```
> > Constant *CElt = CV->getAggregateElement(i);
> > if (isa<ConstantExpr>(CElt))
> >   continue;
> > // TODO: If a select condition element is undef, we can demand from
> > // either side. If one side is known undef, choosing that side would
> > // propagate undef.
> > if (CElt->isNullValue())
> >   DemandedLHS.clearBit(i);
> > else
> >   DemandedRHS.clearBit(i);
> > 
> > ```
> I haven't been following the details of this review, but that code specifically guards against ConstantExpr. Is there some other constant-derived possibility that we failed to account for?
> 
> The code here looks safe since it is explicitly checking for constant 0 or -1, but it doesn't account for the possibility of an undef element in the mask. 
> 
> I'm not sure what we want the result to be in that case. Either way, we probably want a regression test to account for that (and a test with a constant expression, so we're sure we don't crash on that?).
I'm happy to add a test case for both constexpr, and undef.  I specifically *do not* want to be aggressive about undef at this time though.  That opens up a bunch of complexity since we need to be *consistent* about how we interpret an undef mask element, and I'd really rather separate that into it's own patch.  

p.s. That TODO comment in Select looks wrong.  We already interpret undef as falling into the else.  I'm not sure that's correct, but that's a separate patch and a separate discussion.  

p.p.s. In a future patch, abstracting out a helper function for this masked demanded bit splitting operation is planned.  


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

https://reviews.llvm.org/D57372