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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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);
----------------
rogfer01 wrote:
> 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?
Please don't use ISD::ADDC here; now that we're using ADDCARRY, we shouldn't produce that node at all on ARM.  (LowerADDC_ADDE_SUBC_SUBE should have been removed as part of D35192. ARMISD::ADDC is a different node which is still useful.)


https://reviews.llvm.org/D35635