This is an archive of the discontinued LLVM Phabricator instance.

[Constants] Extend support for scalable-vector splats
ClosedPublic

Authored by frasercrmck on May 31 2021, 10:53 AM.

Details

Summary

This patch extends the various "isXXX" functions of the Constant class
to include scalable-vector splats.

In several "isXXX" functions, code that was separately inspecting
ConstantVector and ConstantDataVector was unified to use
getSplatValue, which already includes support for said splats.

In the varous "isNotXXX" functions, code was added to check whether the
scalar splat value -- if any -- satisfies the predicate.

An extra fix for isNotMinSignedValue was included, as it previously
crashed when passed a scalable-vector type because it unconditionally
cast to FixedVectorType

These changes address numerous missed optimizations, a compiler crash
mentioned above and -- perhaps most egregiously -- an infinite loop in
InstCombine due to the compiler breaking canonical form when it failed
to pick up on a splat in a select instruction.

Test cases have been added to cover as many of these functions as
possible, though existing coverage is slim; it doesn't appear that there
are any in-tree uses of Constant::isNegativeZeroValue, for example.

Diff Detail

Event Timeline

frasercrmck created this revision.May 31 2021, 10:53 AM
frasercrmck requested review of this revision.May 31 2021, 10:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2021, 10:53 AM
frasercrmck added inline comments.May 31 2021, 10:58 AM
llvm/test/Transforms/InstCombine/sub.ll
862

This isn't combined to xor as above because the pattern in visitSub uses m_ImmConstant which matches Constant but explicitly not ConstantExpr. Still, it's a call to isNotMinSignedValue before it, checking that we don't mis-optimize.

Matt added a subscriber: Matt.Jun 3 2021, 8:41 AM
RKSimon added inline comments.Jun 7 2021, 1:03 AM
llvm/lib/IR/Constants.cpp
204

I'm not really convinced it'd make a difference but we're not consistent at checking for splats before/after the FixedVectorType only tests - do you think we should be?

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
931 ↗(On Diff #348841)

Could this be easily handled in a separate patch?

llvm/test/Transforms/InstCombine/sub.ll
862

Maybe mention that in a comment just above?

frasercrmck marked an inline comment as done.Jun 7 2021, 2:17 AM
frasercrmck added inline comments.
llvm/lib/IR/Constants.cpp
204

Fair point. The lack of consistency I've introduced stemmed from the different styles going on in these functions in order to minimize changes. The functions that early-exited when !Fixed I added in the splat check before, else I added it after.

Checking for splats is adding another loop over the fixed elements so I'm leaning towards making splats come consistently afterwards, and so removing the !Fixed early-exit checks. That wouldn't penalize the fixed-length loops since they can early-exit if they detect the opposite of what they're checking for.

I guess in theory that would allow us to then only check for splats for scalable vectors, though that may look confusing and is adding extra complexity to the reader at (presumably) no benefit to compilation times.

Sound reasonable?

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
931 ↗(On Diff #348841)

I'm concerned I won't be able to find a test case for this/that, depending on which goes in first, but yes logically they should be distinct changes so I'll give it a go.

llvm/test/Transforms/InstCombine/sub.ll
862

Aye good idea. I may make a note to look into that later. m_ImmConstant is surprisingly rare so we could probably go through them all and see if there's not a reason to "upgrade" them.

frasercrmck marked an inline comment as done.
  • rebase
  • add test fixme comment
  • normalize check order to fixed-length then (scalable) splat
  • consistently remove "this->" from these methods
frasercrmck marked an inline comment as done.Jun 7 2021, 2:30 AM
frasercrmck added inline comments.
llvm/lib/IR/Constants.cpp
204

I've added that now to show what I'm meaning.

  • slice out change to dyn_castNegVal
  • update test with FIXME accordingly
frasercrmck added inline comments.
llvm/test/Transforms/InstCombine/sub.ll
842

@RKSimon by removing the change to dyn_castNegVal I wasn't able to find a good test case for isNotMinSignedValue so I just left this as a FIXME here. This is now addressed in D103801 (which can go in after - I similarly wasn't able to find a test case for it without this patch).

RKSimon accepted this revision.Jun 7 2021, 6:10 AM

LGTM - cheers

This revision is now accepted and ready to land.Jun 7 2021, 6:10 AM
frasercrmck edited the summary of this revision. (Show Details)Jun 7 2021, 6:43 AM
This revision was landed with ongoing or failed builds.Jun 7 2021, 6:46 AM
This revision was automatically updated to reflect the committed changes.