[LLVMdev] Question about DAGCombiner::MatchRotate function

[JinGu Kang]

Hi all,

While I test 
"clang-tests/gcc-4_2-testsuite/src/gcc.c-torture/execute/20020226-1.c", 
I faced something wrong with "DAGCombiner::MatchRotate" function.

This function tries to consume some patterns and generate "ROTL" or 
"ROTR" dag node as following comments:

"DAGCombier::MatchRotate" function in DAGCombiner.cpp
Pattern1
// fold (or (shl (*ext x), (*ext y)),
//          (srl (*ext x), (*ext (sub 32, y)))) ->
//   (*ext (rotl x, y))
// fold (or (shl (*ext x), (*ext y)),
//          (srl (*ext x), (*ext (sub 32, y)))) ->
//   (*ext (rotr x, (sub 32, y)))

pattern2
// fold (or (shl (*ext x), (*ext (sub 32, y))),
//          (srl (*ext x), (*ext y))) ->
//   (*ext (rotl x, y))
// fold (or (shl (*ext x), (*ext (sub 32, y))),
//          (srl (*ext x), (*ext y))) ->
//   (*ext (rotr x, (sub 32, y)))

In order to do this, code checks whether target supports this dag 
operation with specific type(LHS's type) through 
"TLI.isOperationLegalOrCustom" function. I guess the reason is following 
"LegalizeType" can not promote "ROTL" and "ROTR".

The problem is that code does not check this with new specific type 
"LArgVT" and "RArgVT" while processing above patterns. How do you think 
about this? I think checking code is needed. I have attached simple 
patch to fix it. Please review this.

Thanks,
JinGu Kang
-------------- next part --------------
Index: lib/CodeGen/SelectionDAG/DAGCombiner.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/DAGCombiner.cpp	(revision 191902)
+++ lib/CodeGen/SelectionDAG/DAGCombiner.cpp	(working copy)
@@ -3415,12 +3415,16 @@
         //   (*ext (rotr x, (sub 32, y)))
         SDValue LArgExtOp0 = LHSShiftArg.getOperand(0);
         EVT LArgVT = LArgExtOp0.getValueType();
-        if (LArgVT.getSizeInBits() == SUBC->getAPIntValue()) {
-          SDValue V =
-              DAG.getNode(HasROTL ? ISD::ROTL : ISD::ROTR, DL, LArgVT,
-                          LArgExtOp0, HasROTL ? LHSShiftAmt : RHSShiftAmt);
-          return DAG.getNode(LHSShiftArg.getOpcode(), DL, VT, V).getNode();
-        }
+        bool HasROTRWithLArg = TLI.isOperationLegalOrCustom(ISD::ROTR, LArgVT);
+        bool HasROTLWithLArg = TLI.isOperationLegalOrCustom(ISD::ROTL, LArgVT);
+        if (HasROTRWithLArg || HasROTLWithLArg) {
+          if (LArgVT.getSizeInBits() == SUBC->getAPIntValue()) {
+            SDValue V =
+                DAG.getNode(HasROTLWithLArg ? ISD::ROTL : ISD::ROTR, DL, LArgVT,
+                            LArgExtOp0, HasROTL ? LHSShiftAmt : RHSShiftAmt);
+            return DAG.getNode(LHSShiftArg.getOpcode(), DL, VT, V).getNode();
+          }     
+        }     
       }
     }
   } else if (LExtOp0.getOpcode() == ISD::SUB &&
@@ -3444,11 +3448,15 @@
         //   (*ext (rotr x, (sub 32, y)))
         SDValue RArgExtOp0 = RHSShiftArg.getOperand(0);
         EVT RArgVT = RArgExtOp0.getValueType();
-        if (RArgVT.getSizeInBits() == SUBC->getAPIntValue()) {
-          SDValue V =
-              DAG.getNode(HasROTR ? ISD::ROTR : ISD::ROTL, DL, RArgVT,
-                          RArgExtOp0, HasROTR ? RHSShiftAmt : LHSShiftAmt);
-          return DAG.getNode(RHSShiftArg.getOpcode(), DL, VT, V).getNode();
+        bool HasROTRWithRArg = TLI.isOperationLegalOrCustom(ISD::ROTR, RArgVT);
+        bool HasROTLWithRArg = TLI.isOperationLegalOrCustom(ISD::ROTL, RArgVT);
+        if (HasROTRWithRArg || HasROTLWithRArg) {
+          if (RArgVT.getSizeInBits() == SUBC->getAPIntValue()) {
+            SDValue V =
+                DAG.getNode(HasROTRWithRArg ? ISD::ROTR : ISD::ROTL, DL, RArgVT,
+                            RArgExtOp0, HasROTR ? RHSShiftAmt : LHSShiftAmt);
+            return DAG.getNode(RHSShiftArg.getOpcode(), DL, VT, V).getNode();
+          }
         }
       }
     }