[PATCH] D21299: [Codegen Prepare] Swap commutative binops before splitting branch condition.

[Tim Northover via llvm-commits]

t.p.northover added a comment.

> FWIW, this heuristic is not assigning any branch probabilities based on the range size, it only ranks the commutative binary operands based on the generic assumption that if we have two conditions a != 0 and b == 2, it is more likely that a != 0 than b == 2


And I still think that's not obviously true. Integers actually used often take a very limited number of values, and this seems like a common idiom to me:

  int res = some_func();
  if (res < 0)
    llvm_unreachable("WTF happened");
  else if (res == 0)
    [...]

> Earnestly, the regressions were minor (except for perlbench which was -2.5%) compared to the gains that I did not spend time looking for the reason.


I don't mind that you didn't investigate the other regressions, I worry about the experimental soundness of including mcf in the analysis deciding whether this actually is an optimization. For example https://en.wikipedia.org/wiki/Test_set.

<rant>
But this is actually why I try to stay well clear of benchmarking discussions (unless asked, as here). I strongly suspect most of what we do around data gathering is fundamentally unprincipled and probably unsound, and would make actual statisticians and experimental designers claw their eyes out with a rusty spoon.

Unfortunately, I don't know enough to say precisely where we're going wrong or how to fix it. So I mostly just pretend it's not happening and keep out of those areas.
</rant>

To be clear, I'm not really trying to block this. I was asked what I thought and replied.


http://reviews.llvm.org/D21299