This is an archive of the discontinued LLVM Phabricator instance.

[PassManager][PhaseOrdering] lower expects before running simplifyCFG
ClosedPublic

Authored by spatel on Apr 9 2021, 9:32 AM.

Details

Summary

If we run passes before lowering llvm.expect intrinsics to metadata, then those passes have no way to act on the hints provided by llvm.expect. SimplifyCFG is the known offender, and we made it smarter about profile metadata in D98898.

In the motivating example from https://llvm.org/PR49336 , this means we were ignoring the recommended method for a programmer to tell the compiler that a compare+branch is expensive. This change appears to solve that case - the metadata survives to the backend, the compare order is as expected in IR, and the backend does not do anything to reverse it.

We make the same change to the old pass manager to keep things synchronized.

Diff Detail

Event Timeline

spatel created this revision.Apr 9 2021, 9:32 AM
spatel requested review of this revision.Apr 9 2021, 9:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2021, 9:32 AM

I think we should try to keep at least the general pipeline ordering in sync for a little while longer still.
Looks fine in general.

spatel updated this revision to Diff 336520.Apr 9 2021, 10:53 AM
spatel edited the summary of this revision. (Show Details)

Patch updated:
Make the same change to the old pass manager.

lebedev.ri accepted this revision.Apr 9 2021, 10:59 AM

Looks good to me.

This will likely have an impact on the performance (duh),
but since this just improves how much of an use we make of the user hints,
the effect will wary with the hints/codebase, and should be fixed there.

This revision is now accepted and ready to land.Apr 9 2021, 10:59 AM
This revision was landed with ongoing or failed builds.Apr 12 2021, 9:24 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Apr 12 2021, 10:12 AM

This seems to break clang tests: http://45.33.8.238/linux/43899/step_7.txt

Please take a look and revert for now if it takes a while to fix.

nikic added a comment.Sep 9 2021, 1:24 PM

FYI this seems to break expect intrinsics in rust (https://github.com/rust-lang/rust/issues/88767). It looks like the IR generated by rust requires the SimplifyCFG+SROA+EarlyCSE sequence that was previously used in order to bring the expect intrinsics into a form that LowerExpectIntrinsic understands.

FYI this seems to break expect intrinsics in rust (https://github.com/rust-lang/rust/issues/88767). It looks like the IR generated by rust requires the SimplifyCFG+SROA+EarlyCSE sequence that was previously used in order to bring the expect intrinsics into a form that LowerExpectIntrinsic understands.

The lowering of llvm.expect in the optimizer is very fragile. Either we need to:
(1) alter the front-end so expect is placed closer to its user (I have no idea if that is feasible)
(2) enhance -lower-expect to look through complicated sequences
(3) run -lower-expect multiple times -- and in different modes, so it doesn't immediately kill expects that it doesn't match up to branches

In the rust example, we have this unoptimized sequence:

  %2 = call i1 @llvm.expect.i1(i1 %a, i1 false), !dbg !10
  %3 = zext i1 %2 to i8, !dbg !10
  store i8 %3, i8* %0, align 1, !dbg !10
  %4 = load i8, i8* %0, align 1, !dbg !10, !range !11
  %_2 = trunc i8 %4 to i1, !dbg !10
  br label %bb1, !dbg !10

bb1:                                              ; preds = %start
  br i1 %_2, label %bb2, label %bb3, !dbg !10

We need both -simplifycfg (to get the branch into the same block as the expect) and -early-cse (to eliminate the round-trip through memory and the casts). Otherwise, we just drop the expect instead of transforming it into metadata with -lower-expect.

ojeda added a subscriber: ojeda.Apr 27 2022, 11:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2022, 11:34 AM
Herald added a subscriber: StephenFan. · View Herald Transcript