[llvm-commits] [llvm] r122090 - in /llvm/trunk: lib/CodeGen/SelectionDAG/DAGCombiner.cpp test/CodeGen/X86/x86_64-mul-by-const.ll

[dalej]

On Dec 18, 2010, at 1:53 PM, Chris Lattner wrote:
>> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Fri Dec 17 15:45:49 2010
>> @@ -3171,6 +3171,26 @@
>>                       DAG.getConstant(c1 + c2, N1.getValueType()));
>>  }
>> 
>> +  // fold (srl (trunc (srl x, c1)), c2) -> 0 or (trunc (srl x, (add c1, c2)))
>> +  // This is only valid if the OpSizeInBits + c1 = size of inner shift
> 
> Please end the comment with a ".".  Do we already do this for arithmetic shifts, and shift left (with extension instead of trunc) as well?

It is not valid for arithmetic right shifts; the trunc is ANDing off sign bits, not zeroes.  It is not done for shift left; I can add that.  Although I'm not sure how it could arise from real code.

>> +  if (N1C && N0.getOpcode() == ISD::TRUNCATE &&
>> +      N0.getOperand(0).getOpcode() == ISD::SRL &&
>> +      N0.getOperand(0)->getOperand(1).getOpcode() == ISD::Constant) {
> 
> Please use isa<ConstantSDNode>(N0.getOperand(0)->getOperand(1)) which makes the cast<> more obviously right.

OK, but this is following other code in the area.

>> +    uint64_t c1 = 
>> +      cast<ConstantSDNode>(N0.getOperand(0)->getOperand(1))->getZExtValue();
>> +    uint64_t c2 = N1C->getZExtValue();
> 
> This can break if the value is > i64.  Instead of doing this on getZExtValue(), I'd suggest doing the math on APInt's.

These are shift counts, not the value being shifted.  I think it's reasonable to assume number of bits per word is less than 18446744073709551616.  There is lots of nearby code that does so.

>> +    EVT InnerShiftVT = N0.getOperand(0)->getOperand(1).getValueType();
>> +    uint64_t InnerShiftSize = InnerShiftVT.getScalarType().getSizeInBits();
>> +    if (c1 + OpSizeInBits == InnerShiftSize) {
>> +      if (c1 + c2 >= InnerShiftSize)
>> +        return DAG.getConstant(0, VT);
>> +      return DAG.getNode(ISD::TRUNCATE, N0->getDebugLoc(), VT,
> 
> Please add a short comment explaining what you're doing here.  What is the "c1 + OpSizeInBits == InnerShiftSize" check doing?

The comment without the period explains it.  I'll move it down, I guess.

>> +; RUN: llc < %s -mtriple=x86_64-apple-darwin | FileCheck %s
>> +; Formerly there were two shifts.  8771012.
> 
> Please add a rdar:// prefix so that grep will pick it up, thanks!
> 
> -Chris
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101220/7800ac55/attachment.html>