[PATCH] D69038: [InstCombine] Fix miscompile bug in canEvaluateShuffled

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:1069
+    case Instruction::SRem:
+      // Avoid introducing integer div/rem by undef to avoid undefined behavior.
+      for (int i = 0, e = Mask.size(); i != e; ++i)
----------------
Does this apply to FP too? (i'm not seeing that in langref)
If it does, then the comment needs updating.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:1071
+      for (int i = 0, e = Mask.size(); i != e; ++i)
+        if (Mask[i] == -1)
+          return false;
----------------
This `-1` always looks so weird to me.
It would be really good to cleanup it everywhere one day to proper `SHUFFLE_UNDEF_INDEX` or something.


================
Comment at: llvm/test/Transforms/InstCombine/shufflevector-div-rem.ll:4
 
 ; This test case was added as a reproducer for a miscompile, where instcombine
 ; introduced an
----------------
bjope wrote:
> Not sure if I need to pre-commit the test case (?).
> If so, then maybe these comments describing the test cases should be updated to tell that it shows the bad result in the pre-commit.
This shows diff so it's okay in principle.
Tests for other changed opcodes? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69038