[PATCH] D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo

[Abinav Puthan Purayil via Phabricator via llvm-commits]

abinavpp added a comment.

In D106139#2909632 <https://reviews.llvm.org/D106139#2909632>, @lebedev.ri wrote:

> In D106139#2909346 <https://reviews.llvm.org/D106139#2909346>, @abinavpp wrote:
>
>> @lebedev.ri Can you confirm the revision you require in this?
>
>
>
>> Is there an issue with the code in this patch and/or do you want us to move this to inst-combine?
>> I still agree with @arsenm that this sort of thing might need to be done in both the places, in which case I don't see any issue in the order in which we push for it.
>
> I agree that we might need to do this in both the places, emphasis is mine,
> but mainly this should be done in middle-end, and afterwards the back-end fold
> should be motivated by an end-to-end test showing the missed optimization.

That makes sense. My hesitation in moving this to inst-combine immediately is because it works fine in DAG-combine. Given the fact that I'm new to LLVM and that this is my first pull request, my hesitation might be going in the wrong direction. I'll initiate a pull request for the same in inst-combine. I'll address the review comments here as well since we might need this in the future.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8740
 
+  if (SDValue Overflow = combineSRXToOverflow(N))
+    return Overflow;
----------------
craig.topper wrote:
> I should have phrased by question better. Is there any reason to check this in visitSRA or can we just let it get converted to SRL and matched in visitSRL?
I had no idea that the ashr inputs were getting combined from visitSRL() instead of visitSRA(). I can see that SRA is changed to SRL in TargetLowering::SimplifyDemandedBits(). From looking at the handling of SRA in SimplifyDemandedBits(), I can see that it transforms it to SRL whenever it can claim that the input's sign-bit will always be zero. I'm not sure if we'll ever need this transformation in visistSRA(). I have removed it now.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8817
+  SDValue UAddO = DAG.getNode(
+      ISD::UADDO, SDLoc(N0), DAG.getVTList(N0NoZextLHS.getValueType(), MVT::i1),
+      N0NoZextLHS, N0NoZextRHS);
----------------
craig.topper wrote:
> i1 is likely not a legal type. Do we need to check that we're before legalize types?
I have made the exit condition to check LegalTypes instead of LegalOperations. In that case, do we need the (!TLI.isOperationLegalOrCustom(ISD::UADDO)) check above?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106139/new/

https://reviews.llvm.org/D106139