[PATCH] D73555: [SVE] Fix bug in simplification of scalable vector instructions

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4486
+  if (!Scalable && Op0Const && Op1Const)
     return ConstantFoldShuffleVectorInstruction(Op0Const, Op1Const, Mask);
 
----------------
ctetreau wrote:
> efriedma wrote:
> > ConstantFoldShuffleVectorInstruction should be fine?  Or is this temporary to work around some other issue?
> It could be made to be fine with the current implementation that uses a Constant*; The beginning of this function checks `if (isa<UndefValue>(Mask))`, and returns an undef. This currently contains a bug that I just now realized that I failed to address, but it's fixable. However, once https://reviews.llvm.org/D72467 is merged, it will be unsalvageable due to the fact that the mask is passed as an array of int, and to determine if it's an undef, you must walk the array. I wonder if it's worth fixing, since your patch seems destined to land soon.
With new shuffles, we'll eventually want this to work, but sure, we can leave it out for now.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4529
         OpShuf->getMask()->getSplatValue())
       return Op0;
 
----------------
ctetreau wrote:
> efriedma wrote:
> > Can you rearrange this code so you can put the early return earlier?
> I did not rearrange the code because the current implementation forms an ordering of which simplification should be taken, and I assumed that this order was selected on purpose.
It didn't look like the transforms overlapped, but maybe it's more complicated than I thought at first glance?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73555