[PATCH] D53236: [SelectionDAG] swap select_cc operands to enable folding

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:18326
+
+  if ((Fold || Swap) && TLI.getBooleanContents(N0.getValueType()) ==
+       TargetLowering::ZeroOrOneBooleanContent && (!LegalOperations ||
----------------
labrinea wrote:
> Shouldn't `TLI.getBooleanContents(N0.getValueType())` be `TLI.getBooleanContents(getSetCCResultType(N0.getValueType()))` instead, or it doesn't matter?
Given that we're using getSetCCResultType(N0.getValueType()) when we create the SCC value below, that seems right. But that should be a separate clean-up step either before or after this patch. I'm not sure if there's a way to expose that bug in a test though.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:18326-18328
+  if ((Fold || Swap) && TLI.getBooleanContents(N0.getValueType()) ==
+       TargetLowering::ZeroOrOneBooleanContent && (!LegalOperations ||
+       TLI.isOperationLegal(ISD::SETCC, N0.getValueType()))) {
----------------
spatel wrote:
> labrinea wrote:
> > Shouldn't `TLI.getBooleanContents(N0.getValueType())` be `TLI.getBooleanContents(getSetCCResultType(N0.getValueType()))` instead, or it doesn't matter?
> Given that we're using getSetCCResultType(N0.getValueType()) when we create the SCC value below, that seems right. But that should be a separate clean-up step either before or after this patch. I'm not sure if there's a way to expose that bug in a test though.
clang-format?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:18360-18362
+    return DAG.getNode(ISD::SHL, DL, N2.getValueType(), Temp,
+        DAG.getConstant(N2C->getAPIntValue().logBase2(), SDLoc(Temp),
+                        getShiftAmountTy(Temp.getValueType())));
----------------
clang-format?


https://reviews.llvm.org/D53236