[llvm-commits] [llvm] r169951 - in /llvm/trunk: lib/CodeGen/SelectionDAG/DAGCombiner.cpp test/CodeGen/X86/store_op_load_fold.ll

[Eli Friedman]

On Mon, Jan 7, 2013 at 10:46 AM, Manman Ren <mren at apple.com> wrote:
>
> On Dec 21, 2012, at 6:27 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
>
>> On Tue, Dec 11, 2012 at 5:13 PM, Manman Ren <mren at apple.com> wrote:
>>> Author: mren
>>> Date: Tue Dec 11 19:13:50 2012
>>> New Revision: 169951
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=169951&view=rev
>>> Log:
>>> DAGCombine: clamp hi bit in APInt::getBitsSet to avoid assertion
>>>
>>> rdar://12838504
>>>
>>> Modified:
>>>    llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>    llvm/trunk/test/CodeGen/X86/store_op_load_fold.ll
>>>
>>> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=169951&r1=169950&r2=169951&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
>>> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Tue Dec 11 19:13:50 2012
>>> @@ -7433,7 +7433,8 @@
>>>     // start at the previous one.
>>>     if (ShAmt % NewBW)
>>>       ShAmt = (((ShAmt + NewBW - 1) / NewBW) * NewBW) - NewBW;
>>> -    APInt Mask = APInt::getBitsSet(BitWidth, ShAmt, ShAmt + NewBW);
>>> +    APInt Mask = APInt::getBitsSet(BitWidth, ShAmt,
>>> +                                   std::min(BitWidth, ShAmt + NewBW));
>>>     if ((Imm & Mask) == Imm) {
>>>       APInt NewImm = (Imm & Mask).lshr(ShAmt).trunc(NewBW);
>>>       if (Opc == ISD::AND)
>>
>> Are you sure the math here is actually correct?  This looks really
>> suspicious, especially without a comment explaining how the math is
>> supposed to work.
> Hi Eli,
>
> Sorry for my late response. My change was to clamp the high bit of Mask to not exceed the actual APInt's high bit.
> Is that incorrect?

I'm not sure it's wrong, but at the very least, it's not obviously
correct, given the complicated math used to compute ShAmt.  In the
context of this code, what does it actually mean if ShAmt + NewBW is
greater than BW?  Does it actually make sense perform narrowing in
that situation?

-Eli