[llvm-dev] Folding zext from i1 into PHI nodes with only zwo incoming values.

[Sanjay Patel via llvm-dev]

I don't see any downside for the other users of shouldChangeType(), so I've
cleaned up the patch and posted it for review:
https://reviews.llvm.org/D29336

On Tue, Jan 31, 2017 at 9:02 AM, Björn Steinbrink <bsteinbr at gmail.com>
wrote:

>
> 2017-01-31 16:59 GMT+01:00 Sanjay Patel <spatel at rotateright.com>:
>
>>
>>
>> On Tue, Jan 31, 2017 at 3:09 AM, Björn Steinbrink <bsteinbr at gmail.com>
>> wrote:
>>
>>> Hi Sanjay,
>>>
>>> 2017-01-30 22:22 GMT+01:00 Sanjay Patel <spatel at rotateright.com>:
>>>
>>>> My minimal patch idea is to ease the restriction in ShouldChangeType
>>>> because i1 is special. I tried the patch below, and it works on the
>>>> example...and nothing in 'make check' failed. :)
>>>>
>>>
>>> Yeah, that would work for me as well, I just wasn't sure about the
>>> implications that has. Simply making FoldPHIArgOpIntoPHI act like
>>> FoldPHIArgZextsIntoPHI seemed like the safer option to me, but I wanted
>>> feedback on it before creating a PR. Do you want to go ahead with that
>>> minimal approach and create a PR yourself?
>>>
>>
>> Your idea probably has the minimal impact while still fixing your problem
>> case...although the relationships between FoldPHIArgOpIntoPHI /
>> FoldPHIArgZextsIntoPHI / FoldOpIntoPhi are confusing.
>>
>> I think a patch for ShouldChangeType makes sense on its own (and makes
>> any direct changes to the FoldPHI* functions unnecessary for your example).
>> I'll try some more experiments with that patch to look for unintended
>> consequences.
>>
>
> OK, I'll wait for your results then
>
>
>> When you say "PR", do you mean a bugzilla report or a patch proposal on
>> Phab?
>>
>
> I meant a patch proposal on Phab, I'm too used to GitHub terminology by
> now, sorry.
>
> Cheers,
> Björn
>
>
>>
>>
>>>
>>>
>>> Björn
>>>
>>>
>>>> Index: lib/Transforms/InstCombine/InstructionCombining.cpp
>>>> ===================================================================
>>>> --- lib/Transforms/InstCombine/InstructionCombining.cpp    (revision
>>>> 293485)
>>>> +++ lib/Transforms/InstCombine/InstructionCombining.cpp    (working
>>>> copy)
>>>> @@ -88,12 +88,12 @@
>>>>
>>>>  /// Return true if it is desirable to convert an integer computation
>>>> from a
>>>>  /// given bit width to a new bit width.
>>>> -/// We don't want to convert from a legal to an illegal type for
>>>> example or from
>>>> -/// a smaller to a larger illegal type.
>>>> +/// We don't want to convert from a legal to an illegal type or from a
>>>> smaller
>>>> +/// to a larger illegal type. Width of '1' is always treated as a
>>>> legal type.
>>>>  bool InstCombiner::ShouldChangeType(unsigned FromWidth,
>>>>                                      unsigned ToWidth) const {
>>>> -  bool FromLegal = DL.isLegalInteger(FromWidth);
>>>> -  bool ToLegal = DL.isLegalInteger(ToWidth);
>>>> +  bool FromLegal = FromWidth == 1 ? true :
>>>> DL.isLegalInteger(FromWidth);
>>>> +  bool ToLegal = ToWidth == 1 ? true : DL.isLegalInteger(ToWidth);
>>>>
>>>>    // If this is a legal integer from type, and the result would be an
>>>> illegal
>>>>    // type, don't do the transformation.
>>>> @@ -110,7 +110,7 @@
>>>>
>>>>  /// Return true if it is desirable to convert a computation from
>>>> 'From' to 'To'.
>>>>  /// We don't want to convert from a legal to an illegal type for
>>>> example or from
>>>> -/// a smaller to a larger illegal type.
>>>> +/// a smaller to a larger illegal type. i1 is always treated as a
>>>> legal type.
>>>>  bool InstCombiner::ShouldChangeType(Type *From, Type *To) const {
>>>>    assert(From->isIntegerTy() && To->isIntegerTy());
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170131/c09bf06f/attachment.html>