Page MenuHomePhabricator

[AArch64] Add pass to enable additional comparison optimizations by CSE
Needs ReviewPublic

Authored by sdmitrouk on Aug 25 2014, 1:35 AM.

Details

Reviewers
Jiangning
Summary

This pass tries to make consecutive compares of values use same operands to
allow CSE pass to remove duplicated instructions. For this it analyzes
branches and adjusts comparisons with immediate values by converting:

  • GE -> GT
  • GT -> GE
  • LT -> LE
  • LE -> LT

and adjusting immediate values appropriately. It basically corrects two
immediate values towards each other to make them equal.

Diff Detail

Event Timeline

sdmitrouk updated this revision to Diff 12893.Aug 25 2014, 1:35 AM
sdmitrouk retitled this revision from to [AArch64] Add pass to enable additional comparison optimizations by CSE.
sdmitrouk updated this object.
sdmitrouk edited the test plan for this revision. (Show Details)
sdmitrouk added a reviewer: Jiangning.
sdmitrouk added a subscriber: Unknown Object (MLST).
sdmitrouk updated this revision to Diff 12898.Aug 25 2014, 4:39 AM

Add parenthesis around one of expressions.

Jiangning edited edge metadata.Aug 25 2014, 8:11 PM

Hi Sergey,

Now the patch looks good to me! I'll be OK if you commit.

Thanks,
-Jiangning

sdmitrouk updated this revision to Diff 12939.Aug 25 2014, 11:24 PM
sdmitrouk edited edge metadata.

Full diff, the second one was incomplete.

Is this strictly target-specific?

(I'm not suggesting this work be blocked,) I'm wondering if at least some part of this can be implemented in a target-independent way. If so, I would ask that a bug be submitted to continue the discussion.

Chad

Chad,

Is this strictly target-specific?

I guess not, other targets can benefit of similar optimizations as well.

It depends on how lowering is done for that targets. E.g. on x86_64 it
is fine as is, although there is some code to handle similar
cases of logical operations (which wasn't called in my tests). Some
other targets might be in the same situation as AArch64.

One possible generalization is to add target-independent "driver"
pass, which will operate on machine blocks, say, via new methods of
TargetInstrInfo. Such pass would contain only main orchestration
logic (e.g. main loop). Actually, not that much code to share among
different targets. Is it enough to create target-independent pass? If
yes, I'll file a bug.

Sergey

In D5044#11, @sdmitrouk wrote:

Chad,

Is this strictly target-specific?

I guess not, other targets can benefit of similar optimizations as well.

Rather, is the implementation necessarily target-specific? I imagine other targets would benefit, but I'm asking if we could have a more generic solution.

Actually, not that much code to share among
different targets. Is it enough to create target-independent pass? If
yes, I'll file a bug.

Based on your response, my impression is that this requires a mostly target-specific implementation. I can't provide an educated argument for or against your approach, so I think the purpose of the bug would be to continue the discussion. I'll leave it to your good judgment to decide if this discussion is really necessary. In the meantime, I have no issue with the current solution.

Chad

Rather, is the implementation necessarily target-specific? I imagine other targets would benefit, but I'm asking if we could have a more generic solution.

I doubt whether more generic solution will work. Here is some context.
Final code heavily depends on how instructions are lowered (details of SelectionDAG construction is just one of things that affect it).
So I guess this optimization must be implementation-specific to be meaningful.

I'll leave it to your good judgment to decide if this discussion is really necessary.

Then I won't submit a bug as chances that it'll get us somewhere are quite low.
Anyway, I'll keep generalization of this in mind. Thanks for bringing this up.

Sergey

Hi Sergey,

I think your fix itself is an independent one, and it should be able to
work well no matter if there is any solution in shared code for all targets
before this pass. So I personally think you can commit unless anybody has
objection.

Thanks,
-Jiangning

2014-09-03 20:28 GMT+08:00 Sergey Dmitrouk <sdmitrouk@accesssoftek.com>:

Ping.

http://reviews.llvm.org/D5044