Page MenuHomePhabricator

[PGO] Skip if an IndirectBrInst critical edge cannot be split
ClosedPublic

Authored by MaskRay on Sep 10 2020, 12:01 AM.

Details

Summary

PGOInstrumentation runs SplitIndirectBrCriticalEdges but some IndirectBrInst
critical edge cannot be split. getInstrBB will crash when calling SplitCriticalEdge, e.g.

int foo(char *p) {
  void *targets[2];
  targets[0] = &&indirect;
  targets[1] = &&end;
for (;; p++)
  if (*p == 7) {
indirect:
    goto *targets[p[1]]; // the self loop is critical in -O
  }
end:
  return 0;
}

Skip such critical edges to prevent a crash.

Diff Detail

Event Timeline

MaskRay created this revision.Sep 10 2020, 12:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2020, 12:01 AM
MaskRay requested review of this revision.Sep 10 2020, 12:01 AM

I believe every other caller ensures that as a precondition.
Why is the existing approach incorrect, and PGOInstrumentation shouldn't be fixed itself?

I believe every other caller ensures that as a precondition.
Why is the existing approach incorrect, and PGOInstrumentation shouldn't be fixed itself?

How do other callers handle such a critical edge? SplitCriticalEdge is a generic API where some cases are already skipped (return nullptr), see isEHPad and other return nullptr in the function.

I believe every other caller ensures that as a precondition.
Why is the existing approach incorrect, and PGOInstrumentation shouldn't be fixed itself?

How do other callers handle such a critical edge?

By doing this check before calling the function?

SplitCriticalEdge is a generic API where some cases are already skipped (return nullptr), see isEHPad and other return nullptr in the function.

I believe every other caller ensures that as a precondition.
Why is the existing approach incorrect, and PGOInstrumentation shouldn't be fixed itself?

How do other callers handle such a critical edge?

By doing this check before calling the function?

SplitCriticalEdge is a generic API where some cases are already skipped (return nullptr), see isEHPad and other return nullptr in the function.

TBN i personally don't care whether all the callers do all these checks, or SplitCriticalEdge() itself does them (latter is likely slower),
but i would think we should be consistent.

MaskRay updated this revision to Diff 291003.Sep 10 2020, 9:43 AM

Move the check from BreakCriticalEdges.cpp to PGOInstrumentation.cpp

lebedev.ri accepted this revision.Sep 10 2020, 9:50 AM

SGTM, thanks

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
810

s/previous/following/

This revision is now accepted and ready to land.Sep 10 2020, 9:50 AM
MaskRay marked an inline comment as done.Sep 10 2020, 10:12 AM
MaskRay added inline comments.
llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
810

I think "previous" is correct. It is SplitIndirectBrCriticalEdges. The function calls SplitIndirectBrCriticalEdges in the very beginning.

This revision was automatically updated to reflect the committed changes.
MaskRay marked an inline comment as done.