Page MenuHomePhabricator

[CodeGenPrepare] Eliminate llvm.expect before removing empty blocks

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



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

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.


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

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.

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.

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