[PATCH] D144116: [DAGCombiner] Avoid converting (x or/xor const) + y to (x + y) + const if benefit is unclear

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon accepted this revision.
RKSimon added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2667
           isConstantOrConstantVector(N0.getOperand(1), /* NoOpaque */ true)) {
-        return DAG.getNode(ISD::ADD, DL, VT,
-                           DAG.getNode(ISD::ADD, DL, VT, N1, N0.getOperand(0)),
-                           N0.getOperand(1));
+        bool NoAddCarry = TLI.isOperationLegal(ISD::ADD, N0.getValueType());
+        ConstantSDNode *Const = dyn_cast<ConstantSDNode>(N0.getOperand(1));
----------------
aqjune wrote:
> RKSimon wrote:
> > aqjune wrote:
> > > RKSimon wrote:
> > > > aqjune wrote:
> > > > > RKSimon wrote:
> > > > > > Just because the ADD isn't legal doesn't mean we're going to end up with ADC instructions
> > > > > Could you elaborate a bit about this please?
> > > > > If it means that a legal ADD can still be splitted into a chain of ADDS/ADCS/ADC, is there a way to find the bitwidth of addition that will not be splitted?
> > > > I think TLI.getTypeToExpandTo should help you ?
> > > Hi, I tried calling TargetLowering's `getTypeToExpandTo`, but it hit this error in the function if the integers' bitwidths were too small:
> > > ```
> > > llvm_unreachable("Type is not legal nor is it to be expanded!");
> > > ```
> > > Such case did not happen in the unit test of this patch, but another unit test (llvm/test/CodeGen/X86/setcc-combine.ll).
> > > 
> > > I think that my current version also deals with the add's bitwidth type because `N0.getValueType()` is passed as the second argument of `isOperationLegal`. What do you think?
> > > 
> > > 
> > I haven't had time to look at this in detail - but my concern is types that will be promoted (i4 etc.) instead of expanded (i128) - won't your legality logic assume they will need ADC as well?
> Thanks. I updated the patch.
> For the vector types I wasn't sure what this patch should do. Do you have any idea how they must be dealt with?
Nothing comes to mind - I think your change should be OK for vector types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144116