[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);
----------------
efriedma wrote:
> 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.)
Thanks @efriedma. Now I see that I should have removed `LowerADDC_ADDE_SUBC_SUBE` in D35192.

That said I fail to understand why we want to update this function in this patch. @jgalenson does keep using `ISD:ADD` (not to confuse it with `ISD::ADDC`) impact the code generation?

I mean, I understand we can use `ISD::ADDCARRY` here but maybe we want to do this in a later patch?


https://reviews.llvm.org/D35635