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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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]])
----------------
spatel wrote:
> 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.
I think that the matcher issue is a bug, and have submitted D54631 to fix it. At least this behavior should be consistent, i.e. either `m_AllOnes()` matches both scalar and vector undef, or it matches neither. Right now it matches vector, but not scalar, which is neither here nor there...

I also checked whether using `<2 x i8> undef` allows us to work around the problem for the purposes of this test, but it does not change the result. I'm assuming that `<2 x i8> <i8 undef, i8 undef>` gets normalized to `<2 x i8> undef` at some point.

Changing the check order so that the undef check comes first would work, but I'd prefer to fix the matcher implementation than have this somewhat subtle workaround.

Regarding commits, I don't have commit access so can't apply those directly. Should I open another patch that includes just the baseline checks?


Repository:
  rL LLVM

https://reviews.llvm.org/D54532