This is an archive of the discontinued LLVM Phabricator instance.

[BFI][CGP] Add limited support for detecting missed BFI updates and fix one in CodeGenPrepare.
ClosedPublic

Authored by hjyamauchi on Apr 3 2020, 10:52 AM.

Details

Summary

This helps detect some missed BFI updates during CodeGenPrepare.

This is debug build only and disabled behind a flag.

Fix a missed update in CodeGenPrepare::dupRetToEnableTailCallOpts().

Diff Detail

Event Timeline

hjyamauchi created this revision.Apr 3 2020, 10:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2020, 10:52 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

There's already -verify, -verify-dom-info, etc.
I wonder if this should follow similar pattern?

davidxl added inline comments.Apr 3 2020, 3:29 PM
llvm/include/llvm/Analysis/BlockFrequencyInfoImpl.h
1467

If there is no mismatch detected at this point, the following check is redundant.

llvm/lib/CodeGen/CodeGenPrepare.cpp
2220

Add assert that BB's frequency >= TailCallBB's freq?

hjyamauchi marked 4 inline comments as done.

Address comment.

llvm/include/llvm/Analysis/BlockFrequencyInfoImpl.h
1467

If there's a valid block in the Other BFI that's not in this BFI, this code is useful, as the comparison goes both ways?

llvm/lib/CodeGen/CodeGenPrepare.cpp
2220

Added one only under this check mode as I think the assert outside this BFI update check mode would be too strong at this point. Note that BlockFrequency avoids an underflow.

BlockFrequency &BlockFrequency::operator-=(BlockFrequency Freq) {
  // If underflow, set frequency to 0.
  if (Frequency <= Freq.Frequency)
    Frequency = 0;
  else
    Frequency -= Freq.Frequency;
  return *this;
}
davidxl added inline comments.Apr 10 2020, 1:22 PM
llvm/include/llvm/Analysis/BlockFrequencyInfoImpl.h
1467

There are two cases:

  1. when the NumberOfValidNodes is different
  2. when the NumberOfValidNodes is the same.

For the first case, the mismatch will be caught earlier. For the second case, in order to be 'mismatched', there must be a validNode in *this not in Other. Otherwise, Other will be a superset of *this, and it will become case #1. THis means the case 2 will also be caught in the previous loop.

hjyamauchi marked 2 inline comments as done.

Address comment.

llvm/include/llvm/Analysis/BlockFrequencyInfoImpl.h
1467

Done.

davidxl added inline comments.Apr 13 2020, 3:16 PM
llvm/lib/CodeGen/CodeGenPrepare.cpp
233

is it useful to extend the verification to earlier passes in the future?

There's already -verify, -verify-dom-info, etc.
I wonder if this should follow similar pattern?

hjyamauchi marked an inline comment as done.Apr 14 2020, 8:31 AM

There's already -verify, -verify-dom-info, etc.
I wonder if this should follow similar pattern?

The flag name was renamed from "check" to "verify".

llvm/lib/CodeGen/CodeGenPrepare.cpp
233

It would be (but it's not easy though.)

lebedev.ri added a comment.EditedApr 14 2020, 9:06 AM
In D77417#1980889, @yamauchi wrote:

There's already -verify, -verify-dom-info, etc.
I wonder if this should follow similar pattern?

The flag name was renamed from "check" to "verify".

Hmm, maybe i should be more verbose.
My question is, we already have some sudo-passes (-verify, -verify-dom-info)
that can be run after any pass, that verify that internal invariants still hold
at the time they are run. I may be wrong, but it seems like
this is adding a similar consistency check, but it is hardcoded to one particular
position of the pipeline - before CGP. So i'm wondering if i'm misreading that,
and if not whether it should be generalized so that BFI consistency can be checked
after any pass of choice.

Noting that the "updatedness" of BFI *during a pass* is currently only on a best effort basis, what this patch is intended to provide is to have a way to check how well BFI is updated during CGP. It's sort of a middle ground between teaching CGP to preserve BFI (which is hard) and having no way to assess how well it can preserve BFI. Because its updatedness could still have performance consequences.

As I see it, this is a bit different from -verify/-verify-dom-info in that unlike -verify, it cannot be a separate pass as it needs to look at the state of BFI right before it gets erased at the end of CGP (as CGP isn't declared to preserve BFI), and unlike -verify-dom-info, Pass:verifyAnalysis isn't a good place to check it in as Pass:verifyAnalysis is called on preserved analyses only. Though we might be able to extend the pass manager interface to have a place for it (maybe checkNotPreservedAnalysis?) if it's really a good idea. I'm not sure if it's mature enough at this point to be generalized.

This revision is now accepted and ready to land.Apr 23 2020, 10:22 AM
This revision was automatically updated to reflect the committed changes.