Remove possible loop creation in DAGCombiner

[deadal nix]

Alternatively, we can define a function like getSelectPredicateType with 2
argument: the SetCC type and the selected argument size and use that.

2014-09-27 17:28 GMT-07:00 deadal nix <deadalnix at gmail.com>:

> Ok so you'll face the same issue line 11422 when selecting between 32 bits
> pointer size.
>
> Wouldn't it be possible to add a pattern in the td files to handle that
> case precisely ?
>
> 2014-09-27 11:56 GMT-07:00 Matt Arsenault <arsenm2 at gmail.com>:
>
>
>> On Sep 26, 2014, at 1:48 AM, deadal nix <deadalnix at gmail.com> wrote:
>>
>> Which target ? I can try to figure out a solution. As per submitted test
>> cases, this do not affect X86 or X86-64 int that very scenario, the 32 bit
>> select is generated properly.
>>
>>
>> Not an in tree target, AMDIL
>>
>>
>> 2014-09-25 13:57 GMT-07:00 Matt Arsenault <arsenm2 at gmail.com>:
>>
>>>
>>> On Sep 25, 2014, at 1:42 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>>>
>>> > ----- Original Message -----
>>> >> From: "deadal nix" <deadalnix at gmail.com>
>>> >> To: "llvm-commits" <llvm-commits at cs.uiuc.edu>
>>> >> Sent: Thursday, September 25, 2014 12:09:07 AM
>>> >> Subject: Remove possible loop creation in DAGCombiner
>>> >>
>>> >>
>>> >>
>>> >>
>>> >> DAGCombiner can create loop in the DAG when you sext the result of a
>>> >> setcc, depending on the target.
>>> >>
>>> >> This diff remove the call to DAG.getSExtOrTrunc that create the loop.
>>> >> This call used to be necessary for x86 backend, but it doesn't look
>>> >> necessary anymore. I've added tests to ensure it doesn't.
>>> >
>>> > -        EVT SelectVT = getSetCCResultType(VT);
>>> > -        return DAG.getSelect(DL, VT,
>>> > -                             DAG.getSExtOrTrunc(SetCC, DL, SelectVT),
>>> > -                             NegOne, DAG.getConstant(0, VT));
>>> > -
>>> > +        return DAG.getSelect(DL, VT, SetCC,
>>> > +                          NegOne, DAG.getConstant(0, VT));
>>> >       }
>>> >
>>> > It seems clear to me that creating an SIGN_EXTEND inside of
>>> visitSIGN_EXTEND could easily lead to a loop, and we should avoid that.
>>> However, getSExtOrTrunc might also have been providing a TRUNC, and if the
>>> SIGN_EXTEND really would be needed, we may just want to abort the
>>> transformation all together. For one thing, I don't think that it can be
>>> different unless VT != N0.getOperand(0).getValueType().
>>>
>>> I do need the trunc behavior, for the case of selecting between 32-bit
>>> values based on a 64-bit comparison on a target which has the width of the
>>> setcc type change depending on the width of the compared operands
>>>
>>> >
>>> > In short, you're proposing to change target-independent code, and I'd
>>> be more conservative. Why don't you just disable the transformation when
>>> SelectVT.bitsGT(SetCC.getValueType()) is true (which I believe is the
>>> condition under which we'd get the SIGN_EXTEND).
>>>
>>> I agree, and what I was planning on trying when I got back to fixing
>>> that patch.
>>>
>>> >
>>> > -Hal
>>> >
>>> >>
>>> >>
>>> >>
>>> >> _______________________________________________
>>> >> llvm-commits mailing list
>>> >> llvm-commits at cs.uiuc.edu
>>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> >>
>>> >
>>> > --
>>> > Hal Finkel
>>> > Assistant Computational Scientist
>>> > Leadership Computing Facility
>>> > Argonne National Laboratory
>>> > _______________________________________________
>>> > llvm-commits mailing list
>>> > llvm-commits at cs.uiuc.edu
>>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140927/0c817d6d/attachment.html>