[llvm-commits] [PATCH, PING] Peephole Infrastructure improvements and (ARM, T, T2) TSTrr optimizations

[Gabor Greif]

On 11/17/10, Evan Cheng <evan.cheng at apple.com> wrote:
> Hi Gabor,
>
> I've made some minor changes in this area. Can you update your patch again?

Hi Evan,

yes I have updated the patch and uploaded it to
<http://codereview.appspot.com/2781041/>.
You can get hold of the raw patch by

cd llvm
curl http://codereview.appspot.com/download/issue2781041_6001.diff | patch

Please note that some Thumb1 functionality is lost because of your
reordering of passes. The TSTrr is now a CMPri and so it does not get
eliminated. I did not try to fix this yet. The Thumb1 tests thus just
capture the status quo.

> I have some other questions / concerns:
>
> 1.  I have trouble understanding MaxOpportunity and CmpOpportunity. Can you
> add more comments? Please try to eliminate the use of template. It feels
> over-designed and doesn't really fit in TargetInstrInfo.

You seem to agree with Bill here. I think I have a solution that can
satisfy us all. Will get to it in the next few days. My design was
driven by performance considerations, but I see that currently the
cycles are rather burned in other parts, so a more conventional
approach is feasible.

> 2. TrackCopyEquivalences is target independent code. Please move it out of
> ARM. Again, the use of template seems unnecessary.

Yeah, constant propagation should do it. I tried to get the semantics
right first, I can move it around later. Btw. I am not sure whether
the pattern ANDrr...TSTrr still arises on Thumb1. I have to
investigate.

> 3. I'm concerned with the compile time impact of FindCorrespondingAnd. Have
> you measured it?

IIRC it has a cutoff after 5 iterations. I did not measure the effect
but my gut feeling is that the opportunity itself arises seldom and
the algo runs in hot cache, so the impact should be small. But
peephole opts are costly and do not run at -O0, right?

> 4. Can you provide more test cases that shows the benefit of your patch?

It is aimed at untagging operations. Especially with
'switch'-optimization that follows the 'and'. These will definitely
occur a gazillion times when LLVM is JITting on ARM :-)
Later I hope to extend the mechanisms to all targets. See
<http://llvm.org/bugs/show_bug.cgi?id=8125> for all my collected
"wisdom".

>
> Some questions:
> 5. Are we missing opportunities? What about xor? One thing I noticed we are
> always folding (cmp (xor l, r), 0) to teq. That's actually a potential
> pessimization in case the xor has multiple uses. Instead, isel should leave
> it as xor + cmp 0 and let the peephole pass away the cmp.

I see you have tackled this transform to pessimize less. I am aware of
it, but want to get to a solid foundation first.

Cheers,

     Gabor

PS: feel free to comment the codereview issue directly, but I'll
transfer your comments-so-far too.

>
> Evan
>
>
>
> On Oct 29, 2010, at 12:59 AM, Gabor Greif wrote:
>
>> My response below, forgot to add all recipients :-(
>>
>> On 10/29/10, Bill Wendling <wendling at apple.com> wrote:
>>> Sorry about the long response time. A few general comments:
>>>
>>> • Watch out for consistent spacing.
>>> • You should use "llvm::next" instead of "next". The latter conflicts
>>> with
>>> an "std" name (if I recall correctly).
>>> • You call 'new' on the Opportunity classes in a situation where it
>>> expects
>>> to return a "bool" value. What's going on here?
>>>
>>> -bw
>>
>>
>>
>> Hi Bill,
>>
>> thanks for the review!
>>
>> Spacing: I am not aware of problems with my recent patch
>> http://codereview.appspot.com/2781041/ (also attached to the [PING]
>> mail. I added spaces after 'new' and before '(' in my working copy.
>>
>> llvm::next: I never have seen conflicts, Googled and found this:
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40497. Will change for
>> reasons of caution.
>>
>> Returning 'new (Space) MaskOpporunity..." etc. in bool context. It is
>> just a shorthand for these two lines:
>> new (Space) MaskOpporunity...; // fill in closure
>> return true;  // yes we can proceed with the closure
>> But I can make it explicit if you wish. Do you? Okay to commit?
>>
>> Cheers,
>>
>>   Gabor
>>>
>>> On Oct 25, 2010, at 1:28 AM, Gabor Greif wrote:
>>>
>>>> Ping!
>>>>
>>>> Attached the diff against recent trunk.
>>>>
>>>> Cheers,
>>>>
>>>>   Gabor
>>>>
>>>> On 10/21/10, Gabor Greif <ggreif at gmail.com> wrote:
>>>>> Hi all,
>>>>>
>>>>> the last weeks I've been working on a flexible infrastructure for
>>>>> peephole optimizations, which is potentially target independent and
>>>>> extensible without needing interface changes.
>>>>>
>>>>> The result of my work is attached. It moves all current ARM peepholes
>>>>> over to the new architecture and adds TSTrr-related optimizations too.
>>>>>
>>>>> The ordering and forward referencing of functions is still suboptimal,
>>>>> but this is only done to keep the patch size manageable. I plan to
>>>>> reorder in a cleanup commit after this patch has landed. Also some
>>>>> currently freestanding functions will become methods.
>>>>>
>>>>> You can also see the code in its entirety here:
>>>>> <http://llvm.org/viewvc/llvm-project/llvm/branches/ggreif/peephole-infrastructure/>
>>>>>
>>>>> Feedback is welcome.
>>>>>
>>>>> Cheers,
>>>>>
>>>>>   Gabor
>>>>>
>>>> <peep-infra.patch>
>>>
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>