Page MenuHomePhabricator

CodeGen: Placement: Apply triangle heuristic more aggressively at O3.
AbandonedPublic

Authored by iteratee on Mar 7 2017, 5:15 PM.

Details

Reviewers
davidxl
Summary

The triangle tail duplication heuristic can improve performance for small chains
of triangles, but this can cause poorly generated code to perform worse, for
example, one of the mips shift lowerings generates code that looks like this:

if x
  do something that doesn't modify x
if !x
  do something

Requiring 3 in a row avoids this case.
but 2 in a row is likely overly aggressive for O2, at least for now.
I have benchmark data showing this is profitable in the cases where it applies:

No significant performance changes to llvm test-suite. Tiny size increases: 0.027% on the 5 affected benchmarks.

This improves an important internal Google benchmark (protocol buffer serialization) by 2%, with no significant effect on other internal benchmarks.

Diff Detail

Event Timeline

iteratee created this revision.Mar 7 2017, 5:15 PM
iteratee edited the summary of this revision. (Show Details)Mar 7 2017, 5:15 PM

So, this didn't go to the list. I'd abandon it and create a fresh revision with llvm-commits CC'ed directly.

Also, it'd be good to include at least a summary of the benchmark data (especially the LLVM test suite data). You should even be able to include some of our internal benchmark data by looking at how Dehao did it here: http://lists.llvm.org/pipermail/llvm-dev/2017-February/109802.html

Alternatively, you can try cc llvm-commits, then update the description.

iteratee edited the summary of this revision. (Show Details)Mar 14 2017, 5:17 PM
davidxl edited edge metadata.Mar 15 2017, 10:49 AM

Should the mips problem be fixed? I don't see a fundamental reason this needs to be opt level specific.

Should the mips problem be fixed? I don't see a fundamental reason this needs to be opt level specific.

We looked into the mips issue. It is only a problem on mips2 and mips3, which are now 25 years old. The lowering for those targets could be better, but I'll file a bug and let it be.

The issue also goes away with branch coalescing if the mips branch targets are correctly marked as not clobbering the AT register.

Should the mips problem be fixed? I don't see a fundamental reason this needs to be opt level specific.

We looked into the mips issue. It is only a problem on mips2 and mips3, which are now 25 years old. The lowering for those targets could be better, but I'll file a bug and let it be.

The issue also goes away with branch coalescing if the mips branch targets are correctly marked as not clobbering the AT register.

Agreed, I'd very much like to file a bug and punt here. I don't think it's worth the yak shave of fixing the mips port to avoid treating all conditional branches as macro instructions and the branch coalescing patch will be turned on by default soon.

iteratee abandoned this revision.May 5 2017, 9:58 AM