This is an archive of the discontinued LLVM Phabricator instance.

[CodeGenPrepare] Eliminate llvm.expect before removing empty blocks
ClosedPublic

Authored by thejh on Mar 3 2021, 3:11 AM.

Details

Summary

CodeGenPrepare currently first removes empty blocks, then in a loop
performs other optimizations. One of those optimizations is the removal
of call instructions that invoke @llvm.assume, which can create new
empty blocks.

This means that when a branch only contains a call to __builtin_assume(),
the empty branch will survive into MIR, and will then only be
half-removed by MIR-level optimizations (e.g. removing the branch but
leaving the condition intact).

Fix it by eliminating @llvm.expect builtin calls before removing empty
blocks.

Diff Detail

Event Timeline

thejh created this revision.Mar 3 2021, 3:11 AM
thejh requested review of this revision.Mar 3 2021, 3:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2021, 3:11 AM
bkramer accepted this revision.Mar 3 2021, 5:36 AM

This makes sense to me.

llvm/lib/CodeGen/CodeGenPrepare.cpp
627–628

We now have a make_early_inc_range thing to make this pattern less ugly. It can be used in a for range loop.

This revision is now accepted and ready to land.Mar 3 2021, 5:36 AM
thejh added inline comments.Mar 3 2021, 5:56 AM
llvm/lib/CodeGen/CodeGenPrepare.cpp
627–628

I don't think that fits well with resetIteratorIfInvalidatedWhileCalling? The code that I'm moving isn't just deleting the current instruction, but also other instructions that it depends on; my understanding is that the old code is explicitly resetting the iterator in case RecursivelyDeleteTriviallyDeadInstructions ends up throwing away the following instruction because it's recursing through a phi node, or something like that; and I'm preserving that behavior.

By the way, once you think this change is ready to land, please commit it for me - I don't have commit access.

fhahn added a subscriber: fhahn.Mar 3 2021, 6:23 AM
fhahn added inline comments.
llvm/test/Transforms/CodeGenPrepare/remove-assume-block.ll
11

can you also test cases with blocks that contain multiple assumes, as well as tests where assume calls and other instructions are mixed. Also cases where the assumption is a value defined in the function.

thejh updated this revision to Diff 327797.Mar 3 2021, 7:32 AM

added another test

Added another test that should cover fhahn's request.

thejh marked an inline comment as done.Mar 3 2021, 7:37 AM
thejh added inline comments.
llvm/test/Transforms/CodeGenPrepare/remove-assume-block.ll
11

Done - I think the test I've added covers all your requests.