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

[Hal Finkel]

----- Original Message -----
> From: "yi_jiang" <yjiang at apple.com>
> To: "Hal J. Finkel" <hfinkel at anl.gov>
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Tuesday, October 28, 2014 7:04:43 PM
> Subject: Re: [Patch]Do not simplifyLatch for loops where hoisting increments could	result in extra live range
> interferance
> 
> 
> Hi Hal,
> 
> 
> Thanks a lot for your comments.
> I have addressed the first issue. Also the compile time test shows
> that there is no real compile time regressions without preventive
> guard. So I also removed this. Here is the new patch, please let me
> know if any other suggestions. Thank you.

One small thing:

+      Value *IVOpnd = nullptr;

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

+        IVOpnd = I->getOperand(1);

+

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

+        if (IVOpnd && isa<ConstantInt>(IVOpnd))

+          return false;

This last part can be:
  if (IVOpnd)
    return false;
because if IVOpnd is not null, then it must be equal to I->getOperand(1), and we're already inside a block guarded by isa<ConstantInt>(I->getOperand(1)), so checking that isa<ConstantInt>(IVOpnd) is redundant.

Otherwise, LGTM; thanks!

 -Hal

> 
> 
> On Oct 28, 2014, at 8:06 AM, Hal Finkel < hfinkel at anl.gov > wrote:
> 
> 
> 
> 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
> 

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