[llvm-commits] [llvm] r55571 - /llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

[Bill Wendling]

On Aug 30, 2008, at 12:39 PM, Gabor Greif wrote:

>> @@ -2068,10 +2069,8 @@
>>        //   (rotl x, (sub 32, y))
>>        if (ConstantSDNode *SUBC =  
>> cast<ConstantSDNode>(RExtOp0.getOperand(0))) {
>>          if (SUBC->getAPIntValue() == OpSizeInBits) {
>> -          if (HasROTL)
>> -            return DAG.getNode(ISD::ROTL, VT, LHSShiftArg,  
>> LHSShiftAmt).getNode();
>> -          else
>> -            return DAG.getNode(ISD::ROTR, VT, LHSShiftArg,  
>> RHSShiftAmt).getNode();
>> +          return DAG.getNode(HasROTL ? ISD::ROTL : ISD::ROTR, VT,  
>> LHSShiftArg,
>> +                             HasROTL ? LHSShiftAmt :  
>> RHSShiftAmt).getNode();
>>          }
>
> Please note that we have 2 arguments changing depending on the value
> of HasROTL. The comment describing this transformation has identical
> LHSs. Can this be right?
>
The comment is wrong, but the code is correct. Say you have x =  
0xDEADBEEF and y = 4. You want to get out this value:

      (x << 4) | (x >> 28)
   => (0xEADBEEF0) | (0x0000000D)
   => 0xEADBEEFD

The comments say this:

      (rotr x, 4)
   => 0xFDEADBEE

      (rotl x, 28)
   => 0xFDEADBEE

But the code is doing this:

      (rotl x, 4)
   => 0xEADBEEFD

      (rotr x, 28)
   => 0xEADBEEFD

which is correct. I'll update the comments.

>>        }
>>      } else if (LExtOp0.getOpcode() == ISD::SUB &&
>> @@ -2082,10 +2081,8 @@
>>        //   (rotr x, (sub 32, y))
>>        if (ConstantSDNode *SUBC =  
>> cast<ConstantSDNode>(LExtOp0.getOperand(0))) {
>>          if (SUBC->getAPIntValue() == OpSizeInBits) {
>> -          if (HasROTL)
>> -            return DAG.getNode(ISD::ROTL, VT, LHSShiftArg,  
>> RHSShiftAmt).getNode();
>> -          else
>> -            return DAG.getNode(ISD::ROTL, VT, LHSShiftArg,  
>> LHSShiftAmt).getNode();
>> +          return DAG.getNode(ISD::ROTL, VT, LHSShiftArg,
>> +                             HasROTL ? RHSShiftAmt :  
>> LHSShiftAmt).getNode();
>>          }
>
> Here only the last argument to DAG.getNode depends on HasROTL.
> Strange...
> The transformation comment is again deriving different RHSs from an
> identical LHS...
>

Using the same example:

      (x << 28) | (x >> 4)
   => 0xD0000000 | 0x0DEADBEE
   => 0xDDEADBEE

The comments say:

      (rotl x, 4)
   => 0xEADBEEFD

      (rotr x, 28)
   => 0xEADBEEFD

The code is doing this:

      (rotl x, {4,28})

which is obviously wrong. We want it to do this:

      (rotl x, 4)
   => 0xEADBEEFD

      (rotr x, 28)
   => (0xEADBEEFD)

I'll correct the comment and the code.

Thanks for catching these! :-)

-bw