This is an archive of the discontinued LLVM Phabricator instance.

[ArgPromotion] Replace all md uses of promoted values with undef.
ClosedPublic

Authored by fhahn on Aug 3 2020, 5:40 AM.

Details

Summary

Currently, ArgPromotion may leave metadata uses of promoted values,
which will end up in the wrong function, creating invalid IR.

PR33641 fixed this for dead arguments, but it can be also be triggered
arguments with users that are promoted (see the updated test case).

We also have to drop uses to them after promoting them. We need to do
this after dealing with the non-metadata uses, so I also moved the empty
use case to the loop that deals with updating the arguments of the new
function.

Diff Detail

Event Timeline

fhahn created this revision.Aug 3 2020, 5:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2020, 5:40 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
fhahn requested review of this revision.Aug 3 2020, 5:40 AM
aprantl accepted this revision.Aug 3 2020, 10:19 AM
aprantl added inline comments.
llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
413

We could use

llvm::scope_exit RauwUndef([&]() {
// There potentially are metadata uses left for things like
// llvm.dbg.value. Replace them with undef.
I->replaceAllUsesWith(UndefValue::get(I->getType()));
});

if (I->use_empty())
  continue;

I'll leave it up to you to decide whether that's actually better.

This revision is now accepted and ready to land.Aug 3 2020, 10:19 AM
fhahn added inline comments.Aug 3 2020, 10:28 AM
llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
413

Nice, I wasn't aware of scope_exit()! That would allow to get rid of the unfortunate duplication.

The reason we need to drop at the end of the scope is that there is no replaceAllMetadataUsesWith and metadata uses can only be replaced together with regular uses. So alternatively we could also add a replaceAllMetadataUsesWith, but if that's the only place it's probably not worth it. What do you think?