[PATCH] D103421: [Constants] Extend support for scalable-vector splats

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked an inline comment as done.
frasercrmck added inline comments.


================
Comment at: llvm/lib/IR/Constants.cpp:202
+    if (const auto *SplatCFP = dyn_cast_or_null<ConstantFP>(getSplatValue()))
+      return SplatCFP->isFiniteNonZeroFP();
+
----------------
RKSimon wrote:
> 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?
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?


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:931
+        CV->getType()->getScalarType()->isIntegerTy() && CV->getSplatValue())
+      return ConstantExpr::getNeg(CV);
+
----------------
RKSimon wrote:
> Could this be easily handled in a separate patch?
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.


================
Comment at: llvm/test/Transforms/InstCombine/sub.ll:862
+; CHECK-LABEL: @test44scalablevecminval(
+; CHECK-NEXT:    [[SUB:%.*]] = add <vscale x 2 x i16> [[X:%.*]], shufflevector (<vscale x 2 x i16> insertelement (<vscale x 2 x i16> undef, i16 -32768, i32 0), <vscale x 2 x i16> undef, <vscale x 2 x i32> zeroinitializer)
+; CHECK-NEXT:    ret <vscale x 2 x i16> [[SUB]]
----------------
RKSimon wrote:
> frasercrmck wrote:
> > 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.
> Maybe mention that in a comment just above?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103421