[PATCH] D46662: [X86] condition branches folding for three-way conditional codes

[Andrea Di Biagio via llvm-commits]

On Thu, Sep 27, 2018 at 7:46 PM, Rong Xu <xur at google.com> wrote:

>
>
> On Wed, Sep 26, 2018 at 10:57 PM Dávid Bolvanský via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> xbolva00 added a comment.
>>
>> > I suggest to make this pass optional (i.e. make this pass opt-in for
>> subtargets).
>>
>> @xur are you gonna work on this? As you already measured some numbers,
>> you can enable it for haswell, skylake, etc..
>>
>
> Yes. I'm working on this.  I'm using the getprocFamily() Interface in
> X86Subtarget.  Like
>
>  switch (ST.getProcFamily()) {
>     case X86Subtarget::IntelHaswell:
>     case X86Subtarget::IntelBroadwell:
>     case X86Subtarget::IntelSkylake:
>     case X86Subtarget::IntelSKX:
>     case X86Subtarget::IntelCannonlake:
>           ... enable...
> }
>
> But the issue I'm having is for sandybridge or ivybridge processor, it
> returns X86Subtarget::Other.
> I'm have difficulty to include this two subtargets. Am I using the correct
> interface?
>
>
The easiest way to address this problem is to declare a new
SubtargetFeature in x86.td, and add it to subtargets that should run your
new pass.
This would be similarly to what is done for pass FixupLEA; that pass is
only run if the subtarget declares `FeatureSlowLEA` (see X86.td).

That being said, @Craig or @RKSimon might know of a better way to address
your problem

In general, a solution that doesn't require adding (yet another)
SubtargetFeature to X86.td is preferable.
The motivation is that subtarget features have been often (ab)used in the
past to characterize performance aspects of a processor. Ideally, it would
be nice if we could decrease instead their number in future.

On the other hand, there is the precedent of FixupLEAs pass.
But more importantly, this could be a stopgap solution until a better
approach is found.

-Andrea


>>
>>
>> ================
>> Comment at: test/CodeGen/X86/condbr_if.ll:35
>> +
>> +define dso_local i32 @length2_2(i32) {
>> +  %2 = icmp sle i32 %0, 2
>> ----------------
>> We don't need "dso_local" here
>>
>> Will fix this.
>
>>
>> https://reviews.llvm.org/D46662
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180927/12a259aa/attachment.html>