[llvm-commits] [PATCH] InstCombine enhancement

[Eli Friedman]

On Mon, Nov 26, 2012 at 2:08 PM, Shuxin Yang <shuxin.llvm at gmail.com> wrote:
> Hi, Eli:
>
>   Thank you for your feedback. See the following interleaving text.
>
>
> On 11/26/12 1:51 PM, Eli Friedman wrote:
>>
>> On Mon, Nov 26, 2012 at 1:32 PM, Shuxin Yang <shuxin.llvm at gmail.com>
>> wrote:
>>>
>>> Hi, dear all:
>>>
>>>       This change is trying to catch this optimization opportunity (one
>>> of
>>> the two defects reported in rdar://12329730):
>>>
>>>     -----------------------------------------------------
>>>            ((X^C1) >> C2) ^ C3  => (X>>C2) ^ ((C1>>C2)^C3)
>>>       where the subexpression "X ^ C1" has more than one uses, and
>>>       "(X^C1) >> C2" has single use.
>>>    ------------------------------------------------------
>>>
>>>      If the "X ^ C1" has only one use,  InstCombine will distribute the
>>> ">>"
>>> operation
>>> in the "X^C1 >> C2", and the entire expression would be eventually
>>> optimized;
>>> however, if "X ^ C1" has multiple uses,  InstCombine would give up.
>>
>> For new code, please use the matchers defined in
>> llvm/Support/PatternMatch.h (m_Xor etc.), which tend to be more
>> compact than dyn_cast for this sort of matching.
>>
>> Are there any cases where this transformation could make us generate
>> worse code?  If so, how likely do you think it is to happen in
>> practice?
>
> It is difficult to envision a case where this change has negative impact.
> Apparently,
> the expression tree/DAG is shorter than it was, and this xform doesn't
> increase
> the # of instruction.

This transform could lead to more complicated constants, which require
more code to materialize, which I think could lead to worse code.
Probably not a concern in practice.

>>
>> This seems to be a general pattern which could be applied to many
>> operations (at least "&", "|", "+", "-").  It also seems to apply to
>> other shift operations (shl, ashr).  Which of those cases do we handle
>> already?  How hard would it be to generalize this transformation?
>>
>>
> If the sub-expression has only one use, as far as I can tell, the
> InstCombine already does a great
> job even on a contrived tricky expressions.
>
> However, if the sub-expr has multiple use, it simply give up.
>
> I guess my change is one of the few (if not none) cases, where InstCombine
> can take a glimpse
> to the multi-use sub-expression to see if there are opportunities.
>
> Yes, there are other opportunities exposed by taking a quick look into
> multi-use-subexpr;
> we have to take it one step at a time:-).

I'm just pointing it out because we don't want to scatter code all
over instcombine to handle each individual case when you can write a
more general transformation.  And I think we might already have code
for some of those cases.  (instcombine is complicated enough without a
bunch of redundant code.)

-Eli