[PATCH] D54532: [InstructionSimplify] Add support for saturating add/sub

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: test/Transforms/InstSimplify/saturating-add-sub.ll:62
+; CHECK-NEXT:    call void @dummy_vec(<2 x i8> [[A]])
+; CHECK-NEXT:    call void @dummy_vec(<2 x i8> <i8 -1, i8 -1>)
+; CHECK-NEXT:    call void @dummy_vec(<2 x i8> [[A]])
----------------
This result doesn't match the scalar logic for undefs. I didn't step through it, but I'm guessing it's because <i8 undef, i8, undef> matched successfully with m_AllOnes(). Not sure if that should be considered a bug or not. 

Generally, we do folds with undef operands before anything else, so we don't see that potential difference. If that doesn't resolve it, then there's definitely an underlying bug in m_Undef().

Nit 1: I prefer that each test is its own function. I had to step back through the CHECK lines twice to make sure I wasn't seeing things. If each test was its own function, you could interleave scalar/vector, and it would be more obvious if there was a difference between those.

Nit 2: Please commit the tests to trunk with baseline CHECKs as a preliminary commit to the actual code commit. That way, we won't lose the test coverage even if the code patch is reverted for some reason.


Repository:
  rL LLVM

https://reviews.llvm.org/D54532