This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] Match non-uniform constant vectors using predicates (RFC).
ClosedPublic

Authored by RKSimon on Jul 17 2017, 9:54 AM.

Details

Summary

Most combines currently recognise scalar and splat-vector constants, but not non-uniform vector constants.

This is a proposal to introduce a matching mechanism that uses predicates to check against BUILD_VECTOR of ConstantSDNode, as well as scalar ConstantSDNode cases.

I've changed a couple of predicates to demonstrate - the combine-shl changes add currently unsupported cases, while the MatchRotate replaces an existing mechanism.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Jul 17 2017, 9:54 AM
filcab added inline comments.Jul 18 2017, 5:53 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
876

s/aan/a/

880

auto *Cst

889

Don't all the BUILD_VECTOR operands have to have the same type? If so, do we need SVT?
I might be missing something.

917

Is it possible that this succeeds?

918

getValueType() returns the same for LHS and RHS. Is it possible for this test to succeed?

5411

This looks different than what I expected.
What's the behaviour you want?

  • If *one* shift is too big, it triggers UB, so replace with undef
  • or, if *all* the shifts are too big, replace with undef as there's not even one "sane" value

If you want the first (which I'm assuming we'd like, as we'd expose more optimizations), shouldn't you be doing something like this?
if (!matchUnaryPredicate(N1, MatchAllIsOK)) return undef;

Of course, you might prefer the second, but in that case I'd ask for a small comment on why. Either in code or in the commit message.

5447

Re: comment above: This one is ok, as we'll only be able to replace all with zeroes when all the "sums of shift amounts" are out of range.

RKSimon added inline comments.Jul 19 2017, 3:19 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
889

BUILD_VECTOR operands do all have to have the same type, but implicit truncation to the vector's scalar type is permitted which can make things tricky - see isConstOrConstSplat which checks for the same thing.

918

Again, implicit truncation in the BUILD_VECTORs, and since we have 2 separate BUILD_VECTORs they may be doing different things.

5411

This just covers the 'all shifts are known constants and too big' case which is about as much as we can dare to support here - note that DAGCombine doesn't handle 'fold (shl x, undef)'.

I suppose we could have a 'matchAnyUnaryPredicate' version that returns true if any case matches but I don't yet have a use case.

RKSimon updated this revision to Diff 107278.Jul 19 2017, 3:56 AM

Updated based on @filcab's comments (typos and use of auto)

RKSimon marked 2 inline comments as done.Jul 19 2017, 3:57 AM
filcab accepted this revision.Jul 19 2017, 9:57 PM

Ugh, implicit truncate/extend strikes again. I keep forgetting about those.
LGTM, I'm ok with the choice in the use of MatchShiftTooBig, but maybe add a tiny comment mentioning you picked the safer option?

This revision is now accepted and ready to land.Jul 19 2017, 9:57 PM
This revision was automatically updated to reflect the committed changes.