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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:1455
+            DemandedPtrs.clearBit(i);
+          else if (CElt->isAllOnesValue())
+            DemandedPassThrough.clearBit(i);
----------------
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?).


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

https://reviews.llvm.org/D57372