[PATCH] Early Target Dependent DAG Combine

[Quentin Colombet]

Hi Dan,

Thanks for the review.

In fact, I'm already aware of the problem you are reporting, i.e., instcombine disrupts the BFI pattern.
I'm not trying to fix this issue here.

Instead, I was trying to expose the kind of thing that can be achieved with the proposed hook using an in-tree target. 

Unfortunately, I don't know enough the existing problems for in-tree targets to come with a better example, but I'm sure somebody else run into this kind of problems and the proposed hook could have ease their development.

What do you think of the proposed hook?

-Quentin

On Jan 30, 2013, at 1:39 PM, Dan Gohman <dan433584 at gmail.com> wrote:

> The bfiPlusMask case in motivation.ll is not instcombine-canonical. Running instcombine on it similarly disrupts your BFI pattern, so patching dagcombine doesn't seem like it will solve the problem. Is it possible for you to instead generalize the BFI matching code to recognize patterns even when they have been disrupted by this canonicalization?
> 
> Dan
> 
> On Wed, Jan 30, 2013 at 11:41 AM, Quentin Colombet <qcolombet at apple.com> wrote:
> 
> To give a bit of motivation for the proposed patch, here is an example on how it can be used.
> 
> Consider the attached motivation.ll file.
> Compile it using thumb with swift cpu, i.e.,
> llc -march thumb motivation.ll -o - -mcpu=swift  -O3
> 
> The produced output will be similar to attached current.s.
> 
> As you may have noticed, the produced code is really different between the two functions of motivation.ll file, whereas the input code is very similar. In fact, I would have expected for the second function, the same code as the first function plus a AND instruction, i.e., like attached expected.s.
> 
> Using the hook proposed in the previous patch, one can achieve the expected output with the attached earlyDAGCombineARMExample.patch.
> 
> Note: My point is to illustrate the motivation of my previous patch and get reviews for it. Do not spend time reviewing the earlyDAGCombineARMExample.patch, it is not intended to be committed (at least in that thread :)).
> 
> Thanks,
> 
> -Quentin
> 
> 
> 
> 
> 
> 
> On Jan 29, 2013, at 11:05 AM, Quentin Colombet <qcolombet at apple.com> wrote:
> 
>> Hi,
>> 
>> Currently in DAG combiner, we have roughly the following workflow for each Selection DAG node:
>> 1. Target-independent DAG combine
>> 2. Target-specific DAG combine
>> 
>> I'd like to add a target-specific DAG combine pass before any target-independent, i.e.,
>> 1. Target-specific DAG combine
>> 2. Target-independent DAG combine
>> 3. Target-specific DAG combine
>> 
>> Attached is a patch that realizes that.
>> 
>> The patch uses the exact same construction as the existing "late" target-specific DAG combine hook. In other words, it defines three new methods in the base class of target lowering:
>> - One to set which types of SDNode should be processed through the "early" target specific DAG combine
>> - One to query whether a type of SDNode has been marked as should be "early" DAG combined.
>> - One virtual method that is the hook to the "early" DAG combine processing.
>> And adds the call to the related hook in DAGCombiner.
>> 
>> Thanks for your reviews.
>> 
>> -Quentin
>> <earlyTargetSpecificDAGCombine.patch>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130130/693bd50c/attachment.html>