[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]])
----------------
nikic wrote:
> 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?
Thanks - I wasn't sure if you were willing to fix the underlying problem, so that's the reason I suggested the work-around for this patch.

You're correct that <2 x i8> <i8 undef, i8 undef> becomes <2 x i8> undef. That should be somewhere in ConstantFolding.

I will commit the baseline test files on your behalf shortly. Then, you can rebase D54631 and this one.



Repository:
  rL LLVM

https://reviews.llvm.org/D54532