[PATCH] D107552: [InstCombine] Combine lshr of add that intends to get the carry as llvm.uadd.with.overflow

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.
Herald added a subscriber: StephenFan.
Herald added a project: All.

In D107552#2930663 <https://reviews.llvm.org/D107552#2930663>, @dmgreen wrote:

> I was trying out some examples, to see how this performs on ARM architectures. This pattern was relatively rare and didn't come up a lot in the benchmarks I tried (or optimized to be the same performance/codesize).
>
> Testing the specific pattern, the first one I tried was i8's, a type not legal on those archs. It doesn't do great with this (although aarch64 does do OK given the instructions it can use):
> https://godbolt.org/z/hbP445xsT
> i32 looks OK for the same thing. AArch64 even looks better, I think because ARM already uses addcarry and splits things in a way that turns out to be equivalent.
> https://godbolt.org/z/3zbhKPs1e
>
> I was trying to look at across basic block too, but didn't do very well at getting something that wasn't just optimized away:
> i8: https://godbolt.org/z/n8sxv3rqE
> i32: https://godbolt.org/z/1sEa7Wd68
> And then there was also this, but it crashed putting instructions in the wrong places:
> https://godbolt.org/z/48abYPq8M
>
> I have no strong objections to creating uadd_with_overflow in instcombine, but we have not done that in the past and I don't know if I see a huge reason to start now. It appears that we should at least be limiting it to legal types.

It's arguably a worse canonical form since fewer optimizations will understand this (e.g. ScalarEvolution, though not sure it really will matter here).



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:1056
+
+  if (!(isPowerOf2_32(BitWidth) && Ty->isIntegerTy(ShAmt * 2)))
+    return nullptr;
----------------
Demorgan this


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:1059-1060
+
+  if (!DL.isLegalInteger(ShAmt))
+    return nullptr;
+
----------------
I also think a legality check doesn't belong here. I also don't think the legal types in the datalayout is particularly well defined


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:1063
+  Value *X = nullptr, *Y = nullptr;
+  if (!(match(Op0, m_Add(m_ZExt(m_Value(X)), m_ZExt(m_Value(Y)))) &&
+        X->getType()->getScalarSizeInBits() == ShAmt &&
----------------
demorgan


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107552