This is an archive of the discontinued LLVM Phabricator instance.

Simply operands of masked stores and scatters based on demanded elements
ClosedPublic

Authored by reames on Jan 25 2019, 9:46 AM.

Details

Summary

If we know we're not storing a lane, we don't need to compute the lane. This could be improved by using the undef element result to further prune the mask, but I want to separate that into its own change since it's relatively likely to expose other problems.

Question for reviewer: Surely there's a better way to go from an <N x i1> mask to the demanded elements bits? One that works for more than just constant masks?

In the same spirit as https://reviews.llvm.org/D57177. Once these two are in, will extend to gather and masked.load. This is in the broader context of improving vector pointer instcombine under https://reviews.llvm.org/D57140.

Diff Detail

Event Timeline

reames created this revision.Jan 25 2019, 9:46 AM

Would it be better to put this inside InstCombiner::SimplifyDemandedVectorElts so we can also simplify the mask based on the demanded elts from its user(s)?

Would it be better to put this inside InstCombiner::SimplifyDemandedVectorElts so we can also simplify the mask based on the demanded elts from its user(s)?

Is that idiomatic for a void result instruction? I thought SimplifyDemandedVectorElts expected to be given a value of vector type?

Would it be better to put this inside InstCombiner::SimplifyDemandedVectorElts so we can also simplify the mask based on the demanded elts from its user(s)?

Is that idiomatic for a void result instruction? I thought SimplifyDemandedVectorElts expected to be given a value of vector type?

*facepalm* sorry, my brain was in Friday mode and I was thinking gathers/masked-loads.....

I agree some kind of helper function that can extract vector values from various Constant vector forms would be useful - they do keep turning up in code in various guises (and if you do gathers/maskedloads you'll be duplicating the code yet again!).

I agree some kind of helper function that can extract vector values from various Constant vector forms would be useful - they do keep turning up in code in various guises (and if you do gathers/maskedloads you'll be duplicating the code yet again!).

Mind if I do this in a follow up commit? And can you point me to a few places that duplicate this? I'm just getting up to speed on how we handle masks and don't have the code indexed in my head yet. :)

I added the tests with baseline checks; please rebase after rL356283.
This seems ok to me, but let's double-check with @RKSimon about the refactoring options.

include/llvm/Analysis/VectorUtils.h
78

Why is this changing in this patch?

reames updated this revision to Diff 191429.Mar 19 2019, 8:20 PM

Rebase, and factor out some common code.

RKSimon accepted this revision.Mar 20 2019, 6:43 AM

LGTM with a couple of minor comments.

lib/Transforms/InstCombine/InstCombineCalls.cpp
1185

Not sure if its any better but you can get this from DemandedElts.getBitWidth():

APInt UndefElts(DemandedElts.getBitWidth(), 0);

1258

DemandedElts.getBitWidth()

This revision is now accepted and ready to land.Mar 20 2019, 6:43 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 20 2019, 11:47 AM