This is an archive of the discontinued LLVM Phabricator instance.

[GlobalOpt] Extend CleanupPointerRootUsers to handle CE users.
ClosedPublic

Authored by fhahn on Feb 21 2023, 3:12 AM.

Details

Summary

Extend CleanupPointerRootUsers to iterate over a worklist, add users of
constant expressions to the worklist to enable additional cleanups.

Diff Detail

Event Timeline

fhahn created this revision.Feb 21 2023, 3:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2023, 3:12 AM
fhahn requested review of this revision.Feb 21 2023, 3:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2023, 3:12 AM
nikic added inline comments.Feb 21 2023, 5:46 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
247

Do we know that the users here are only cast/GEP?

fhahn updated this revision to Diff 501063.Feb 28 2023, 2:51 AM
fhahn marked an inline comment as done.

Rebase on top of c34936465c5648f825414fe76ccad7a9ad82dddd which adds a test with non GEP constant expressions.

Also update ThinLTO test.

fhahn added inline comments.Feb 28 2023, 2:53 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
247

It could be any constant expression, but would that be a problem? I think the existing code should handle any type of constant expressions already. I added some extra tests with different constant expressions in c34936465c56 and 7f51145b1bc2

nikic added inline comments.Feb 28 2023, 3:04 AM
llvm/lib/Transforms/IPO/GlobalOpt.cpp
247

To give a silly example store i32 0, ptr inttoptr (i64 add (i64 ptrtoint ptr @g to i64, i64 100)) to ptr) could be writing to a different underlying object (without UB if certain conditions are met).

fhahn updated this revision to Diff 501255.Feb 28 2023, 12:10 PM

Limit to GEPOperator CEs for now.

fhahn marked an inline comment as done.Feb 28 2023, 12:13 PM
fhahn added inline comments.
llvm/lib/Transforms/IPO/GlobalOpt.cpp
247

At the moment, earlier global analysis will fail if we encounter such constant expressions, but it is probably safest to just limit things to GEPOperator constant expressions here for now to avoid potentially breaking things in the future if the analysis may change. Updated the patch and also removed the Visited set which shouldn't be needed with the restricted version.

nikic accepted this revision.Mar 1 2023, 5:56 AM

LGTM

This revision is now accepted and ready to land.Mar 1 2023, 5:56 AM
This revision was landed with ongoing or failed builds.Mar 2 2023, 1:13 AM
This revision was automatically updated to reflect the committed changes.
fhahn marked an inline comment as done.

Hi,

The followiong starts crashing with this patch:

opt -passes="globalopt" bbi-80589.ll -o /dev/null

I get

opt: ../include/llvm/Support/Casting.h:708: auto llvm::cast_if_present(Y *) [X = llvm::Constant, Y = llvm::Value]: Assertion `isa<X>(Val) && "cast_if_present<Ty>() argument of incompatible type!"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: ../../main-github/llvm/build-all/bin/opt -passes=globalopt bbi-80589.ll -o /dev/null
 #0 0x0000000002e62f98 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (../../main-github/llvm/build-all/bin/opt+0x2e62f98)
 #1 0x0000000002e60b2e llvm::sys::RunSignalHandlers() (../../main-github/llvm/build-all/bin/opt+0x2e60b2e)
 #2 0x0000000002e63616 SignalHandler(int) Signals.cpp:0:0
 #3 0x00007f4e70ae7630 __restore_rt sigaction.c:0:0
 #4 0x00007f4e6e22e387 raise (/lib64/libc.so.6+0x36387)
 #5 0x00007f4e6e22fa78 abort (/lib64/libc.so.6+0x37a78)
 #6 0x00007f4e6e2271a6 __assert_fail_base (/lib64/libc.so.6+0x2f1a6)
 #7 0x00007f4e6e227252 (/lib64/libc.so.6+0x2f252)
 #8 0x00000000026aac05 llvm::ConstantExprKeyType::ConstantExprKeyType(llvm::ConstantExpr const*, llvm::SmallVectorImpl<llvm::Constant*>&) crtstuff.c:0:0
 #9 0x00000000026aa90e llvm::ConstantUniqueMap<llvm::ConstantExpr>::MapInfo::getHashValue(llvm::ConstantExpr const*) crtstuff.c:0:0
#10 0x000000000269e6b5 llvm::ConstantUniqueMap<llvm::ConstantExpr>::remove(llvm::ConstantExpr*) crtstuff.c:0:0
#11 0x0000000002690340 llvm::Constant::destroyConstant() (../../main-github/llvm/build-all/bin/opt+0x2690340)
#12 0x000000000316fc59 CleanupPointerRootUsers(llvm::GlobalVariable*, llvm::function_ref<llvm::TargetLibraryInfo& (llvm::Function&)>) GlobalOpt.cpp:0:0
#13 0x000000000316a12a processInternalGlobal(llvm::GlobalVariable*, llvm::GlobalStatus const&, llvm::function_ref<llvm::TargetTransformInfo& (llvm::Function&)>, llvm::function_ref<llvm::TargetLibraryInfo& (llvm::Function&)>, llvm::function_ref<llvm::DominatorTree& (llvm::Function&)>) GlobalOpt.cpp:0:0
#14 0x0000000003169a7d processGlobal(llvm::GlobalValue&, llvm::function_ref<llvm::TargetTransformInfo& (llvm::Function&)>, llvm::function_ref<llvm::TargetLibraryInfo& (llvm::Function&)>, llvm::function_ref<llvm::DominatorTree& (llvm::Function&)>) GlobalOpt.cpp:0:0
#15 0x0000000003167aca optimizeGlobalsInModule(llvm::Module&, llvm::DataLayout const&, llvm::function_ref<llvm::TargetLibraryInfo& (llvm::Function&)>, llvm::function_ref<llvm::TargetTransformInfo& (llvm::Function&)>, llvm::function_ref<llvm::BlockFrequencyInfo& (llvm::Function&)>, llvm::function_ref<llvm::DominatorTree& (llvm::Function&)>, llvm::function_ref<void (llvm::Function&)>, llvm::function_ref<void (llvm::Function&)>) GlobalOpt.cpp:0:0
#16 0x0000000003165c82 llvm::GlobalOptPass::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (../../main-github/llvm/build-all/bin/opt+0x3165c82)
#17 0x00000000030815fd llvm::detail::PassModel<llvm::Module, llvm::GlobalOptPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) crtstuff.c:0:0
#18 0x00000000027d75db llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (../../main-github/llvm/build-all/bin/opt+0x27d75db)
#19 0x000000000073b8b3 llvm::runPassPipeline(llvm::StringRef, llvm::Module&, llvm::TargetMachine*, llvm::TargetLibraryInfoImpl*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::StringRef, llvm::ArrayRef<llvm::PassPlugin>, llvm::opt_tool::OutputKind, llvm::opt_tool::VerifierKind, bool, bool, bool, bool, bool, bool) (../../main-github/llvm/build-all/bin/opt+0x73b8b3)
#20 0x0000000000749eb2 main (../../main-github/llvm/build-all/bin/opt+0x749eb2)
#21 0x00007f4e6e21a555 __libc_start_main (/lib64/libc.so.6+0x22555)
#22 0x0000000000734b10 _start (../../main-github/llvm/build-all/bin/opt+0x734b10)
Abort (core dumped)

The original unreduced reproducer randomly crashed this way and sometimes it instead hit

opt: ../lib/IR/ConstantsContext.h:616: void llvm::ConstantUniqueMap<llvm::ConstantExpr>::remove(ConstantClass *) [ConstantClass = llvm::ConstantExpr]: Assertion `I != Map.end() && "Constant not found in constant table!"' failed.

so I'm not sure if there is something more lurking here.