Page MenuHomePhabricator

[Verifier] Verify all blockaddress constants left over have at least one user.
Needs ReviewPublic

Authored by fhahn on Jul 19 2019, 2:45 AM.

Details

Summary

Some passes currently do not properly clean up blockaddress constants,
which limits further optimizations, as the blocks remain used by the
constant and remain marked as having their address taken.

With this patch and D64936, the following tests are failing:

LLVM :: Transforms/ConstProp/basictest.ll
LLVM :: Transforms/IndirectBrExpand/basic.ll

Event Timeline

fhahn created this revision.Jul 19 2019, 2:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2019, 2:45 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

Hello @fhahn , it looks like more tests are failing on tip 7df225dfc25adc8371188dc57f3adf300b0bd697 when I tested on x86_64:

Failing Tests (4):
    LLVM :: CodeGen/X86/retpoline.ll
    LLVM :: CodeGen/X86/speculative-load-hardening-indirect.ll
    LLVM :: Transforms/ConstProp/basictest.ll
    LLVM :: Transforms/IndirectBrExpand/basic.ll
llvm/lib/IR/Verifier.cpp
4985

Is it possible to add the name of the block/function to this assert? When looking at basic.ll I wasn't sure if the block in question was bb or entry of @test2().

I'm not sure this is something we can reasonably enforce without making big changes in other areas of the compiler. For example, we call eraseFromParent() all over the place, and some large fraction of those calls could eliminate the last use of a blockaddress.

Places that need this soft of cleanup for global variables usually call Constant::removeDeadConstantUsers(). Not sure if we try to do anything like that for blockaddress constants.

fhahn added a comment.Jul 24 2019, 8:09 AM

I'm not sure this is something we can reasonably enforce without making big changes in other areas of the compiler. For example, we call eraseFromParent() all over the place, and some large fraction of those calls could eliminate the last use of a blockaddress.

Places that need this soft of cleanup for global variables usually call Constant::removeDeadConstantUsers(). Not sure if we try to do anything like that for blockaddress constants.

Yep it is probably not realistic to get everything clean soon given that it also isn't a huge issue. This might surface some obvious cases/places to clean up though :)

Turns out clang can also generate unused block address constants, e.g. for

static a(void) {
b:
  &&b;
}
c() { a(); }

Maybe it would still make sense to add it disabled-by-default?