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

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added inline comments.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:3942
+    // We do not use it in the USUBO case as Value may not be used.
+    Value = DAG.getNode(ISD::ADDC, dl, Op.getValueType(), LHS, RHS).getValue(0);
     OverflowCmp = DAG.getNode(ARMISD::CMP, dl, MVT::Glue, Value, LHS);
----------------
jgalenson wrote:
> rogfer01 wrote:
> > efriedma wrote:
> > > Still the wrong type... you need to call getVTList to get the right type for an ADDC.
> > > 
> > > Also, are you sure you don't want an ARMISD::ADDC, rather than an ISD::ADDC?
> > This comment now looks odd because I had to revert D35192 (still working on it, though).
> > 
> > What was the reason to use a target specific node here? Did you want to save some round trips lowering this or you needed it for better codegen?
> I used the target-specific node here because efriedma suggested it, but using the generic ISD::ADDC does seem to work (it passes all the tests).  Should I change it?  I don't know much about the difference between them.
> 
> It is interesting that this still works after you reverted your patch.
My understanding is that several generic nodes (`ISD::*`) have to be lowered at some point to Arm-specific ones (`ARMISD::*`). `ISD::ADD` and `ISD::SUB` are among those nodes. Using Arm-specific nodes earlier might bring finer control but may miss some of the generic combiners done on generic nodes.

But it is true that when this function is used, the generic node will be found among specific ones (like `ARMISD::CMP`) so generic combiners are unlikely to kick in here, which might be a reason to use the specific one directly and not bothering using the generic one (which I presume will require an extra iteration replacing it with a specific one).

Changing just this case will make this function look a bit odd. If Eli's comment was in the line of "let's make this function only use ARMISD" (instead of `ISD::ADD` and `ISD::SUB`) we can make this change in a later patch and leave this function untouched (except for the comment).

@efriedma am I missing something?


https://reviews.llvm.org/D35635