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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau marked 3 inline comments as done.
ctetreau added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4463
+
+  if (!Scalable) {
+    // Canonicalization: If mask does not select elements from an input vector,
----------------
efriedma wrote:
> Redundant "if"?
The inner computation walks the indices array, so we do need to test it. I suppose I could have put the !Scalable check inside the match conditional, but I don't think that'd be any clearer. I guess it'd make the diff smaller?


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4486
+  if (!Scalable && Op0Const && Op1Const)
     return ConstantFoldShuffleVectorInstruction(Op0Const, Op1Const, Mask);
 
----------------
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.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4529
         OpShuf->getMask()->getSplatValue())
       return Op0;
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73555