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

[sdmitrouk]

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.

[sdmitrouk]

Add parenthesis around one of expressions.

[Jiangning]

Hi Sergey,

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

Thanks,
-Jiangning

[sdmitrouk]

Full diff, the second one was incomplete.

[mcrosier]

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

[sdmitrouk]

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

[mcrosier]

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

[sdmitrouk]

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

[sdmitrouk]

Ping.

[Jiangning]

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 <[e-mail]>:

Ping.

http://reviews.llvm.org/D5044

lib/Target/AArch64/AArch64ConditionOptimizer.cpp

 // Transforms GT -> GE, GE -> GT, LT -> LE, LE -> LT by updating comparison
 // operator and condition code.
 AArch64ConditionOptimizer::CmpInfo AArch64ConditionOptimizer::adjustCmp(
     MachineInstr *CmpMI, AArch64CC::CondCode Cmp) {
   int Opc = CmpMI->getOpcode();
 
   // CMN (compare with negative immediate) is an alias to ADDS (as
   // "operand - negative" == "operand + positive")
-  bool Negative = Opc == AArch64::ADDSWri || Opc == AArch64::ADDSXri;
+  bool Negative = (Opc == AArch64::ADDSWri || Opc == AArch64::ADDSXri);
 
   int Correction = (Cmp == AArch64CC::GT) ? 1 : -1;
   // Negate Correction value for comparison with negative immediate (CMN).
   if (Negative) {
     Correction = -Correction;
   }
 
   const int OldImm = (int)CmpMI->getOperand(2).getImm();