[Patch]Do not simplifyLatch for loops where hoisting increments could result in extra live range interferance

[Hal Finkel]

Hi Yi,

+      if (isa<ConstantInt>(I->getOperand(0)))

+        IVOpnd = I->getOperand(1);

+

+      if (isa<ConstantInt>(I->getOperand(1)))

+        IVOpnd = I->getOperand(0);

I know this seems silly, but it seems like this code will do something silly if given an unsimplified input where both operands are constants (because checking uses of constants will not really give you what you want). I think adding this afterward will make it robust to this situation:

  if (IVOpnd && isa<ConstantInt>(IVOpnd))
    return false;

+        // If the operand has more than 6 uses, we just give up to avoid

+        // compile time issue.

+        if (IVOpnd->getNumUses() > 6)

Can you please define command-line option for this (defaulting to 6 seems fine). I prefer not to have unchangable magic numbers for thresholds in the code like this.

Also, have you seen a compile-time impact if you don't include this cutoff? I'd really prefer that we not introduce cutoffs like this unless they produce a measurable difference. L->contains is a DenseSet lookup, and should be relatively fast. The danger is that you add this cutoff, and the simple cases still work, but you introduce regressions in large, complicated loops that are really hard to track down.

Thanks,
Hal

----- Original Message -----
> From: "yi_jiang" <yjiang at apple.com>
> To: llvm-commits at cs.uiuc.edu
> Sent: Monday, October 27, 2014 5:27:17 PM
> Subject: [Patch]Do not simplifyLatch for loops where hoisting increments could	result in extra live range
> interferance
> 
> 
> 
> Hi,
> 
> This path is to address a typical code as find loop with early exit.
> 
> for ( ; i!=end ; i++) {
> if (i == k)
> break;
> 
> 
> ...
> }
> 
> if (i == end)
> ...
> And the IR is:
> for.cond: ; preds = %for.inc, %entry
> ...
> 
> 
> for.body: ; preds = %for.cond
> %0 = load i32* %coerce.val.ip9, align 4, !tbaa !1
> ...
> br i1 %cmp, label %for.end, label %for.inc // early exit
> 
> 
> for.inc: ; preds = %for.body
> %incdec.ptr.i = getelementptr inbounds i32* %coerce.val.ip9, i64 1
> br label %for.cond
> 
> 
> If we hoist the increment in the for.inc to the for.body then it will
> cause extra live range interference and hurt the performance. So
> this patch will check for uses of the increments operand and do not
> hoist if there are outside users.
> 
> 
> This patch will improve the spec2006 case xalanbmk(ref size) by 10%
> on arm64. We do not see regressions on arm64 and on x86 we see 1%
> regression on the case:
> SingleSource/Benchmarks/BenchmarkGame/fannkuch. The reasons is that
> if the increments in the latch was hoist, it can be combined with
> some other instruction which make it beneficial in this case even
> with live range interference. Thank you for reviewing it.
> 
> 
> -Yi

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory