llvm dev - May 2012 - [LLVMdev] Bug in SUB expansion going back to LLVM 2.6

I found a bug in the expansion code for SUB going back to at least LLVM 2.6 and
still shows up in trunk.
case ISD::SUB: {
    EVT VT = Node->getValueType(0);
    assert(TLI.isOperationLegalOrCustom(ISD::ADD, VT) &&
           TLI.isOperationLegalOrCustom(ISD::XOR, VT) &&
           "Don't know how to expand this subtraction!");
    Tmp1 = DAG.getNode(ISD::XOR, dl, VT, Node->getOperand(1),
               DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), VT));
    Tmp1 = DAG.getNode(ISD::ADD, dl, VT, Tmp2, DAG.getConstant(1, VT));
    Results.push_back(DAG.getNode(ISD::ADD, dl, VT, Node->getOperand(0),
Tmp1));
    break;
  }


The problem is Tmp2 is not initialized and should be Tmp1 instead. This code
only is hit if the architecture does not support 'SUB' and you need to
expand it.

Patch is attached, please commit if good.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120521/c84baa82/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: isd_sub_expand.diff
Type: application/octet-stream
Size: 672 bytes
Desc: isd_sub_expand.diff
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120521/c84baa82/attachment.obj>

On May 21, 2012, at 11:21 AM, Villmow, Micah wrote:
> I found a bug in the expansion code for SUB going back to at least LLVM 2.6
and still shows up in trunk.
> case ISD::SUB: {
>     EVT VT = Node->getValueType(0);
>     assert(TLI.isOperationLegalOrCustom(ISD::ADD, VT) &&
>            TLI.isOperationLegalOrCustom(ISD::XOR, VT) &&
>            "Don't know how to expand this subtraction!");
>     Tmp1 = DAG.getNode(ISD::XOR, dl, VT, Node->getOperand(1),
>                DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()),
VT));
>     Tmp1 = DAG.getNode(ISD::ADD, dl, VT, Tmp2, DAG.getConstant(1, VT));
>     Results.push_back(DAG.getNode(ISD::ADD, dl, VT, Node->getOperand(0),
Tmp1));
>     break;
>   }
>  
>  
> The problem is Tmp2 is not initialized and should be Tmp1 instead. This
code only is hit if the architecture does not support ‘SUB’ and you need to
expand it.
>  
> Patch is attached, please commit if good.
It seems to me that a test case demonstrating bad code would be helpful here.
[ Not that I'm suggesting that the current code is correct… ]

-- Marshall

Marshall Clow     Idio Software   <mailto:mclow.lists at gmail.com>

A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly
moderated down to (-1, Flamebait).
        -- Yu Suzuki

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120521/6b688c40/attachment.html>

Here is a test case:

define i32 @sub_expand_test(i32 %a, i32  %b) nounwind {
entry:
  %tmp5 = sub i32 %b, %a
  ret i32 %tmp5
}

The problem with this is that unless you modified the *ISelLowering.cpp class to
set ISD::SUB to 'Expand', you won't trigger this.

From: Marshall Clow [mailto:mclow.lists at gmail.com]
Sent: Monday, May 21, 2012 11:33 AM
To: Villmow, Micah
Cc: LLVM Dev
Subject: Re: [LLVMdev] Bug in SUB expansion going back to LLVM 2.6

On May 21, 2012, at 11:21 AM, Villmow, Micah wrote:


I found a bug in the expansion code for SUB going back to at least LLVM 2.6 and
still shows up in trunk.
case ISD::SUB: {
    EVT VT = Node->getValueType(0);
    assert(TLI.isOperationLegalOrCustom(ISD::ADD, VT) &&
           TLI.isOperationLegalOrCustom(ISD::XOR, VT) &&
           "Don't know how to expand this subtraction!");
    Tmp1 = DAG.getNode(ISD::XOR, dl, VT, Node->getOperand(1),
               DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), VT));
    Tmp1 = DAG.getNode(ISD::ADD, dl, VT, Tmp2, DAG.getConstant(1, VT));
    Results.push_back(DAG.getNode(ISD::ADD, dl, VT, Node->getOperand(0),
Tmp1));
    break;
  }


The problem is Tmp2 is not initialized and should be Tmp1 instead. This code
only is hit if the architecture does not support 'SUB' and you need to
expand it.

Patch is attached, please commit if good.

It seems to me that a test case demonstrating bad code would be helpful here.
[ Not that I'm suggesting that the current code is correct... ]

-- Marshall

Marshall Clow     Idio Software   <mailto:mclow.lists at gmail.com>

A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly
moderated down to (-1, Flamebait).
        -- Yu Suzuki

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120521/480b8025/attachment.html>

Applied in r157215.

--Owen

On May 21, 2012, at 11:21 AM, "Villmow, Micah" <Micah.Villmow at
amd.com> wrote:
> I found a bug in the expansion code for SUB going back to at least LLVM 2.6
and still shows up in trunk.
> case ISD::SUB: {
>     EVT VT = Node->getValueType(0);
>     assert(TLI.isOperationLegalOrCustom(ISD::ADD, VT) &&
>            TLI.isOperationLegalOrCustom(ISD::XOR, VT) &&
>            "Don't know how to expand this subtraction!");
>     Tmp1 = DAG.getNode(ISD::XOR, dl, VT, Node->getOperand(1),
>                DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()),
VT));
>     Tmp1 = DAG.getNode(ISD::ADD, dl, VT, Tmp2, DAG.getConstant(1, VT));
>     Results.push_back(DAG.getNode(ISD::ADD, dl, VT, Node->getOperand(0),
Tmp1));
>     break;
>   }
>  
>  
> The problem is Tmp2 is not initialized and should be Tmp1 instead. This
code only is hit if the architecture does not support ‘SUB’ and you need to
expand it.
>  
> Patch is attached, please commit if good.
> <isd_sub_expand.diff>_______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120521/e9ca4570/attachment.html>