[PATCH] D35192: [ARM] Use ADDCARRY / SUBCARRY

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added a comment.

Thanks for the review Eli!



================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:9891-9894
+  // If HiAdd is a predecessor of ADDC, the replacement below will create a
+  // cycle.
+  if (AddcNode->isPredecessorOf(HiAdd->getNode()))
+    return SDValue();
----------------
efriedma wrote:
> rogfer01 wrote:
> > This fixes the problem in PR34045.
> > 
> > I wonder if `AddeNode` + `LowAdd` might have the same problem but I don't think it can happen.
> > 
> > The rest of boringssl has built correctly using `-mthumb`, though.
> > I wonder if AddeNode + LowAdd might have the same problem but I don't think it can happen.
> 
> loAdd is a predecessor of AddcNode, which is a predecessor of AddeNode, so I don't think you can run into problems there.   (It's possible for loAdd to use the result of the UMUL_LOHI, but you don't end up with a cycle in that case, just a redundant multiply.)
> 
> I guess this issue only shows up with your patch because it's impossible to create a DAG like that without ADDCARRY transforms?
I think so. Before the ADDCARRY transforms the DAG is slightly simpler at the point this combiner kicks in.


https://reviews.llvm.org/D35192