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

[Evan Cheng]

Hi Gabor,

I've made some minor changes in this area. Can you update your patch again? 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.
2. TrackCopyEquivalences is target independent code. Please move it out of ARM. Again, the use of template seems unnecessary.
3. I'm concerned with the compile time impact of FindCorrespondingAnd. Have you measured it?
4. Can you provide more test cases that shows the benefit of your patch?

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.

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