[PATCH] D35635: Optimize {s,u}{add,sub}.with.overflow on ARM

[Joel Galenson via Phabricator via llvm-commits]

jgalenson added inline comments.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:4484
+  };
+  if (LHS.getOpcode() == ISD::AND &&
+      (isOneConstant(RHS) || isNullConstant(RHS)) &&
----------------
efriedma wrote:
> jgalenson wrote:
> > efriedma wrote:
> > > We should be able to optimize away the AND here using known bits; does that not happen on trunk?
> > It doesn't because ARM doesn't call setBooleanContents.  If there's a way to set that correctly, it would indeed be better.  Do you think we could do that?
> Really?  Wow, that's a big oversight.  We should fix that.
Do you know the correct way to set it?  I tried, but a few tests failed, and I don't know the architecture well enough to know if that was my fault or of the tests just needed to be tweaked.

If we do make that change, then I'll be able to simplify this code, of course.


================
Comment at: test/CodeGen/ARM/su_addsub_overflow.ll:20
+  ; CHECK-NOT: mov
+  ; CHECK: movvc
+}
----------------
jgalenson wrote:
> efriedma wrote:
> > These CHECK lines are not very good; try generating checks with utils/update_llc_test_checks.py.  Also, please commit this first and rebase your patch on top of it; that makes it easy to see what happens to the generated code.
> That tool does make the changes to the testfiles more obvious, but it also generates overly-specific testcases that seem more brittle.  I'll see if I can make a nice middle-ground and update it in a bit.
Okay, I've uploaded both the old and new versions of the test here.  Thanks for that tool, by the way; it's pretty convenient.


https://reviews.llvm.org/D35635