[PATCH] D124481: [llvm][misexpect] Add tests for sample profiling

Tue May 24 18:16:54 PDT 2022

paulkirth added a comment.

In D124481#3534375 <https://reviews.llvm.org/D124481#3534375>, @tejohnson wrote:

> Won't this patch if committed trigger the failure again?

Yes, it will. I'm still trying to figure the situation out, but I wanted to create a case that triggers the crash with the assert, and determine if we can fix the underlying problem before going back to early return.

> Can you use the original failing test case to dig into this more? Looks like it was XFDO from the internal Google bug. Reach out to me internally if you need help reproducing that.

I used that case to determine that the underlying issue is that branch weights are added that have 0 total weight across all branches. I even ran llvm-reduce over the internal example, and it came out remarkably similar to the `misexpect-zero.ll` test.

> I believe it is possible to combine IR and SamplePGO profiles, but it doesn't look like that is the case in the failing test case.

I went back and reviewed the code in SampleProfile, and I think I understand the source of the bug, but I'm not sure exactly how to solve it completely.

When I reimplemented MisExpect, I failed to notice that part of `SampleProfiling` behaves very differently than when I first implemented it in 2019.

The comment in `SampleProfile.cpp` just below the call into misexpect illustrates the problem:

  // OverwriteExistingWeights. In ThinLTO, the profile annotation is done
  // twice. If the first annotation already set the weights, the second pass
  // does not need to set it. With OverwriteExistingWeights, Blocks with zero
  // weight should have their existing metadata (possibly annotated by LTO
  // prelink) cleared.

Looking at the blame, this was changed back in 2021, and I missed that difference when I started working on this again. Previously there was no mention of ThinLTO here, and I failed to account for that scenario and the fact that an existing branch weight may have an origin other that form a lowered `llvm.expect` intrinsic.

An option to work around this is to only do MisExpect checking when `MaxWeight > 0`, which will ensure that at least one branch weight will be non-zero.

It doesn't solve the issue that there are times when a pre-existing weight doesn't imply that it comes from `llvm.expect`, and I really don't have a good answer to how we can address that (and also the fact that you can mix IR and sample profiling).

One option would be to augment branch weight metadata to identify how it was added: via profile or annotation. I'm not convinced that's a great idea, since it would require updates to all the parts of llvm that add branch weights., and increases the size of metadata for a single diagnostic.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124481/new/

https://reviews.llvm.org/D124481