[LLVMdev] Possible bug in ExpandShiftWithUnknownAmountBit

[Javier Martinez]

Hi Eli,

I'm not familiar with how to use SVN that way plus my code is based off 2.4
so I can't compare with TOT. Attached are Unix diff and HTML reports of the
changes to ExpandShiftWithUnknownAmountBit using a different tool (Araxis
Merge). I hope this suffices.

Thanks,
Javier

On Mon, 30 Nov 2009 19:59:26 -0800, Eli Friedman <eli.friedman at gmail.com>
wrote:
> On Mon, Nov 30, 2009 at 7:22 PM, Javier Martinez <javier at jmartinez.org>
> wrote:
>> Hello,
>>
>> I'm working in adding support for 64-bit integers to my target. I'm
using
>> LLVM to decompose the 64-bit integer operations by using 32-bit
registers
>> wherever possible and emulating support where not. When looking at the
>> bit
>> shift decomposition I saw what seems to be a bug in the implementation.
>> The
>> affected function is ExpandShiftWithUnknownAmountBit in
>> LegalizeIntegerTypes.cpp. Below is the original code and the proposed
>> fix.
>> Could someone please review the changes? If they are correct how do I go
>> about submitting a patch?
> 
> [snip]
> 
> Please use "svn diff" to generate proposed patches; the form you used
> is extremely difficult to read.
> 
> -Eli
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20091130/70804e49/attachment.html>
-------------- next part --------------
18c18,19
<   SDValue Amt2 = DAG.getNode(ISD::SUB, dl, ShTy, NVBitsNode, Amt);
---
>   SDValue NVSizeSubAmt = DAG.getNode(ISD::SUB, dl, ShTy, NVBitsNode, Amt);
>   SDValue AmtSubNVSize = DAG.getNode(ISD::SUB, dl, ShTy, Amt, NVBitsNode);
27,28c28,31
<     Lo1 = DAG.getConstant(0, NVT);                  // Low part is zero.
<     Hi1 = DAG.getNode(ISD::SHL, dl, NVT, InL, Amt); // High part from Lo part.
---
>     Lo1 = DAG.getNode(ISD::SHL, dl, NVT, InL, Amt);
>     Hi1 = DAG.getNode(ISD::OR, dl, NVT,
>                       DAG.getNode(ISD::SHL, dl, NVT, InH, Amt),
>                       DAG.getNode(ISD::SRL, dl, NVT, InL, NVSizeSubAmt));
31,34c34,35
<     Lo2 = DAG.getNode(ISD::SHL, dl, NVT, InL, Amt);
<     Hi2 = DAG.getNode(ISD::OR, dl, NVT,
<                       DAG.getNode(ISD::SHL, dl, NVT, InH, Amt),
<                       DAG.getNode(ISD::SRL, dl, NVT, InL, Amt2));
---
>     Lo2 = DAG.getConstant(0, NVT);                           // Low part is zero.
>     Hi2 = DAG.getNode(ISD::SHL, dl, NVT, InL, AmtSubNVSize); // High part from Lo part.
41,42c42,45
<     Hi1 = DAG.getConstant(0, NVT);                  // Hi part is zero.
<     Lo1 = DAG.getNode(ISD::SRL, dl, NVT, InH, Amt); // Lo part from Hi part.
---
>     Lo1 = DAG.getNode(ISD::OR, dl, NVT,
>                      DAG.getNode(ISD::SRL, dl, NVT, InL, Amt),
>                      DAG.getNode(ISD::SHL, dl, NVT, InH, NVSizeSubAmt));
>     Hi1 = DAG.getNode(ISD::SRL, dl, NVT, InH, Amt);
45,48c48,49
<     Hi2 = DAG.getNode(ISD::SRL, dl, NVT, InH, Amt);
<     Lo2 = DAG.getNode(ISD::OR, dl, NVT,
<                      DAG.getNode(ISD::SRL, dl, NVT, InL, Amt),
<                      DAG.getNode(ISD::SHL, dl, NVT, InH, Amt2));
---
>     Lo2 = DAG.getNode(ISD::SRL, dl, NVT, InH, AmtSubNVSize); // Lo part from Hi part.
>     Hi2 = DAG.getConstant(0, NVT);                           // Hi part is zero.
55,57c56,59
<     Hi1 = DAG.getNode(ISD::SRA, dl, NVT, InH,       // Sign extend high part.
<                        DAG.getConstant(NVTBits-1, ShTy));
<     Lo1 = DAG.getNode(ISD::SRA, dl, NVT, InH, Amt); // Lo part from Hi part.
---
>     Lo1 = DAG.getNode(ISD::OR, dl, NVT,
>                      DAG.getNode(ISD::SRL, dl, NVT, InL, Amt),
>                      DAG.getNode(ISD::SHL, dl, NVT, InH, NVSizeSubAmt));
>     Hi1 = DAG.getNode(ISD::SRA, dl, NVT, InH, Amt);
60,63c62,63
<     Hi2 = DAG.getNode(ISD::SRA, dl, NVT, InH, Amt);
<     Lo2 = DAG.getNode(ISD::OR, dl, NVT,
<                       DAG.getNode(ISD::SRL, dl, NVT, InL, Amt),
<                       DAG.getNode(ISD::SHL, dl, NVT, InH, Amt2));
---
>     Lo2 = DAG.getNode(ISD::SRA, dl, NVT, InH, AmtSubNVSize); // Lo part from Hi part.
>     Hi2 = DAG.getConstant(0, NVT);                           // Hi part is zero.
72d71
<