llvm dev - Oct 2013 - [LLVMdev] Question about DAGCombiner::MatchRotate function

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();
+          }
         }
       }
     }

Hi JinGu,

It's normally best to send patches to the llvm-commits list. That's
where most of the reviews happen.
> 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 agree.
> I have attached simple patch to fix it. Please review this.
Which target does this affect (that you know of)? It would be really
good to have a test (like the other .ll files in test/CodeGen/XXX) for
this in the patch.

Cheers.

Tim.

Hi Tim,

I appreciate your response. I faced this error with new architecture so 
I have no idea about correct working example with existing architectures 
on llvm at this moment. Because some patterns can be disappeared or 
changed according to architectures before this dag combining. I am sorry 
for that. If I find a working example, I will commit this. And I will 
send a e-mail to llvm-commits next time. :)

Thanks for your kind response,
JinGu Kang