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

[Chris Lattner]

On Dec 20, 2010, at 11:39 AM, dalej wrote
> 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!

Ok, thanks, please do.

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101225/07bbb546/attachment.html>