In D48056#1132694, @mbrase wrote:

Thanks for reviewing this! I don't have commit access, can someone commit this for me?

LGTM.

Okay, now I understand. Seems like a reasonable extension.

Can you please add a test case?

SGTM!

This make sense to me. LGTM.

In D45374#1064283, @fhahn wrote:

Thanks for the update Chad. There is no need for this change then I think.

Thanks, Eric!

LGTM. Thanks, Geoff. It would be nice to get this into the 6.0.1 release, if at all possible.

In D45092#1053793, @echristo wrote:

Should we instead be translating and passing the option expected?

Update based on Eric's feedback.

In D45374#1063478, @mcrosier wrote:

In D45374#1061368, @fhahn wrote:

Thanks for having a look and sorry for not being clearer. Chad discovered a regression in SPEC2006's h264ref with LTO, caused by this change. The problem was that we peeled off an iteration of a big loop before LTO. That caused the function to be too big for the inliner during LTO, whereas it would be inlined before. We based the peeling decision on a nested condition. With this patch I tried to find a balance between increasing code size and benefits of peeling (simplifying nested conditionals are likely to have less positive impact than top-level ones).

@mcrosier did some more digging and found that we might just want to run simple unrolling before LTO and normal unrolling/peeling during LTO, which makes a sense to me. With that, we would not need this patch (or we could only consider top-level conditionals for "simple" peeling) and IMO that is what we should try to do.

Prior to loop peeling the function we'd like to inlined in h264ref has a single use.

Currently, (non-simple) loop peeling will not peel a loop if it includes a function call that is likely to be inlined (i.e., is not marked with a noinline attribute, has internal linkage and has a single use). This is exactly the case we're dealing with in h264ref, except the function to be inlined isn't marked as internal until the LTO phase of compilation. Thus, one possible approach would be to defer peeling until the LTO phase.

In D45374#1061368, @fhahn wrote:

Thanks for having a look and sorry for not being clearer. Chad discovered a regression in SPEC2006's h264ref with LTO, caused by this change. The problem was that we peeled off an iteration of a big loop before LTO. That caused the function to be too big for the inliner during LTO, whereas it would be inlined before. We based the peeling decision on a nested condition. With this patch I tried to find a balance between increasing code size and benefits of peeling (simplifying nested conditionals are likely to have less positive impact than top-level ones).

@mcrosier did some more digging and found that we might just want to run simple unrolling before LTO and normal unrolling/peeling during LTO, which makes a sense to me. With that, we would not need this patch (or we could only consider top-level conditionals for "simple" peeling) and IMO that is what we should try to do.

In D43876#1060032, @efriedma wrote:

This is exactly the case we're dealing with here except FullPelBlockMotionBiPred isn't marked as internal until the LTO phase of compilation. Thus, one possible approach would be to defer peeling until the LTO phase. After r329392, this can be accomplished with a small change to the pass manager:

ThinLTO doesn't run vectorization or unrolling before link-time; among other reasons, it avoids problems like this. You might want to consider making non-thin LTO work the same way.

In D43876#1059839, @fhahn wrote:

In D43876#1057409, @mcrosier wrote:

In D43876#1057247, @mcrosier wrote:

In D43876#1050460, @fhahn wrote:

@mcrosier I've submitted D44983 for review. It prevents peeling, if we cannot simplify the loop body after peeling. Peeling if we cannot simplify the loop body afterwards is likely not beneficial. It would be great if you could check if that helps in your case. If it's not easy for you to check, I can try and test it myself.

Coming up with some additional heuristics, e.g. based on the number of instructions peeled and not eliminated, should be possible, but without knowing the inlining situation we would probably have to choose a rather arbitrary threshold.

Sure, I'll take a look now and let you know! Sorry I didn't see this sooner.

Unfortunately, D44983 does not fix this case. I'm going to dig into this now. I'll update you once I have some additional findings.

I had a closer look at FullPelBlockMotionBiPred and we peeled off an iteration because we have something like

if (i % 2) {
  ...
  if (i != 0) {...}
 ...
}

Peeling based on those nested conditional is likely to increase the code size too much compared to the benefit. D45374 only considers conditions in blocks that are executed on every iteration.

Thanks everyone for the quick review. I'll commit shortly.

In D43876#1057247, @mcrosier wrote:

In D43876#1050460, @fhahn wrote:

@mcrosier I've submitted D44983 for review. It prevents peeling, if we cannot simplify the loop body after peeling. Peeling if we cannot simplify the loop body afterwards is likely not beneficial. It would be great if you could check if that helps in your case. If it's not easy for you to check, I can try and test it myself.

Coming up with some additional heuristics, e.g. based on the number of instructions peeled and not eliminated, should be possible, but without knowing the inlining situation we would probably have to choose a rather arbitrary threshold.

Sure, I'll take a look now and let you know! Sorry I didn't see this sooner.

In D43876#1050460, @fhahn wrote:

@mcrosier I've submitted D44983 for review. It prevents peeling, if we cannot simplify the loop body after peeling. Peeling if we cannot simplify the loop body afterwards is likely not beneficial. It would be great if you could check if that helps in your case. If it's not easy for you to check, I can try and test it myself.

Coming up with some additional heuristics, e.g. based on the number of instructions peeled and not eliminated, should be possible, but without knowing the inlining situation we would probably have to choose a rather arbitrary threshold.

Still LGTM. Thanks for the fix, Haicheng.

Hi Peter/Rafael,
After this commit, I noticed a ~3.3% regression in SPEC2006/libquantum when compiling with -O3 -flto and the gold-plugin. In short, this change disabled tail duplication during machine block placement as that optimization is only performed when the optlevel is >= Aggressive (and we now default to -O2/Default). In turn, I've started looking into adding function attributes to control the code generation optimization level, per the suggestion in the commit message. Before I get too far into the implementation, I was wondering if there had been some discussion on adding these function attributes? If so, can you point me toward these discussions?

In D43876#1046222, @fhahn wrote:

Hi Chad,

In D43876#1045969, @mcrosier wrote:

Hi Florian,
We identified a 2.15% regression in SPEC2006/h264ref due to this commit. After this change, the FullPelBlockMotionBiPred() function is no longer inlined into the hottest function, BlockMotionSearch(). Previous to this change, the function was inlined because there was a single callsite in the entire program (known only when compiling in LTO) and the original definition could be removed after inlining. However, after loop peeling the callsite of FullPelBlockMotionBiPred() is replicated, which prevents inlining.

I was wondering if we could avoid peeling in this case until we have some type of cost model that can determine if peeling would prevent inlining. Also, after looking at the code (which I can't share here) you might also notice that the amount of code being peeled in this case is fairly large relative to the amount of code being removed from the loop. It might also make sense to have a heuristic that takes code size into consideration when peeling, if that hasn't already been done.

Thoughts?

Thanks for making me aware of this, I originally thought considering MaxPeelCount should help us avoid those cases.

I will follow this up with patches in the following days. I think there are a couple of things that can be done to make the peeling more conservative for now. First, only peel if we can proof that the condition is known to be true in the peeled part and false in the loop (or vice versa). Otherwise we cannot simplify the loop body and peeling is likely not very beneficial. Second, have a simple cost function, that takes the size of the loop body vs the eliminated instructions into account.

Also, D43878, which enables induction variable simplification after peeling is not committed yet, so currently the loop body may not be simplified after peeling, even if it could be.

Cheers,
Florian

Hi Florian,
We identified a 2.15% regression in SPEC2006/h264ref due to this commit. After this change, the FullPelBlockMotionBiPred() function is no longer inlined into the hottest function, BlockMotionSearch(). Previous to this change, the function was inlined because there was a single callsite in the entire program (known only when compiling in LTO) and the original definition could be removed after inlining. However, after loop peeling the callsite of FullPelBlockMotionBiPred() is replicated, which prevents inlining.

LGTM, but formal approval should probably come from someone outside of our group (ping! :).

A few quick comments. Will follow up with a more complete review later this week.

Hi Digeo,
Matt will be out of the office for a few weeks and he asked me to follow up on this patch. Hopefully, the new version addresses all of your concerns.

Address Diego's comments.

ping.

Thanks, Matthias/Jun!

Would it be possible to revert r322917 while we investigate the regressions? We also identified a 3.61% regression in SPEC2006/bzip2, so here's to complete list of regressions we are currently seeing due to this change:

I think we've identified a work around internally, so I'm just going to abandon this patch.

In D42070#976331, @krytarowski wrote:

2.11.3 has been released in 2010, do we support so old releases?

I'd like to suggest that this implementation be included in MachineSink.cpp, but continue to live as a separate pass. The two passes have the same intent, but must be independent passes due to the previously mentioned constrains; I'd almost want to refer to this as the PostRAMachineSink pass... The point being I'd like to have one place where I can see what's sunk pre-RA and what's sunk post-RA.

All of the review feedback has been addressed and I have no additional comments. LGTM. Thanks, Haicheng.

In D40476#936700, @mcrosier wrote:

In D40476#936372, @pirama wrote:

Thanks for the review. Now let's just hope the windows bots stay happy :)

Actually, I just checked and it looks like falkor and saphira were both added as of a few weeks ago. I'll revert this part of the patch shortly.

In D40476#936372, @pirama wrote:

Thanks for the review. Now let's just hope the windows bots stay happy :)

Am I correct in assuming this is going to be a problem for Falkor and Saphira as well? If so, can you add solutions for those as well? Cortex-a57 should be good enough for those targets as well.

Please run clang-format, but otherwise LGTM.

In D39976#925078, @evandro wrote:

Please, let me know if I addressed your concerns before I split the code refactoring in another patch.

The load/store opt pass is already pretty expensive in terms of compile-time. Did you see any compile-time regressions in your testing? Also, what performance results have you collected?

In D39830#921949, @craig.topper wrote:

This solution doesn't seem very general, it won't catch.

double test2(double a, double b, double c, double d) {
  return a + -2.0*b*c*d;
}

The constant can be many layers of multiplies away. Reassociate pushes constants down the tree. Should reassociate be pulling out the negate when it factors the tree?

In D39830#921143, @escha wrote:

what is the purpose of this transform?