[PATCH] D112717: [IR] Replace *all* uses of a constant expression by corresponding instruction

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec accepted this revision.
rampitec added a comment.

LGTM



================
Comment at: llvm/lib/IR/ReplaceConstant.cpp:93
+  for (auto Item : Visited)
+    Item.first->removeDeadConstantUsers();
 }
----------------
hsmhsm wrote:
> rampitec wrote:
> > hsmhsm wrote:
> > > rampitec wrote:
> > > > Why not to remove it immediately?
> > > Consider below example, where C2 uses lds.  In this case, we have two paths, but both the paths have same sequence, that is (C1, C2).  So,  it again causes the same bug which https://reviews.llvm.org/D104425 has fixed. That is, we land up replacing already dead constant expression. 
> > > 
> > > 
> > > ```
> > >                                    I
> > >                                  /  \
> > >                                C1   C1
> > >                               /        \
> > >                              C2        C2
> > > ```
> > > 
> > I mean, dead users are already dead users. In this case you would just call `removeDeadConstantUsers` twice, which shall not be a problem as far as I understand. Just why a separate loop is needed?
> What I mean here is - when we process one of the duplicate paths, constant expression is dead hence it will get removed. Now, when we try to process other remaining path, both C1 and C2 are already visited, and also they are dead by now, which result in compiler crash as below.
> 
> ```
> 0.      Program arguments: /home/mahesha/ROCm/github/llvm/rel/build/bin/opt -S -mtriple=amdgcn-- -passes=amdgpu-lower-module-lds lower-kernel-lds-constexpr.ll -o hsm.ll
>  #0 0x00005565dca163cf PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
>  #1 0x00005565dca13b7d SignalHandler(int) Signals.cpp:0:0
>  #2 0x00007f4cbeccb980 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12980)
>  #3 0x00005565db3699b4 llvm::GetElementPtrInst::Create(llvm::Type*, llvm::Value*, llvm::ArrayRef<llvm::Value*>, llvm::Twine const&, llvm::Instruction*) (/home/mahesha/ROCm/github/llvm/rel/build/bin/opt+0xa0a9b4)
>  #4 0x00005565dc03cd03 llvm::ConstantExpr::getAsInstruction(llvm::Instruction*) const (/home/mahesha/ROCm/github/llvm/rel/build/bin/opt+0x16ddd03)
>  #5 0x00005565dc18896d llvm::convertConstantExprsToInstructions(llvm::Instruction*, std::map<llvm::Use*, std::vector<std::vector<llvm::ConstantExpr*, std::allocator<llvm::ConstantExpr*> >, std::allocator<std::vector<llvm::ConstantExpr*, std::allocator<llvm::ConstantExpr*> > > >, std::less<llvm::Use*>, std::allocator<std::pair<llvm::Use* const, std::vector<std::vector<llvm::ConstantExpr*, std::allocator<llvm::ConstantExpr*> >, std::allocator<std::vector<llvm::ConstantExpr*, std::allocator<llvm::ConstantExpr*> > > > > > >&, llvm::SmallPtrSetImpl<llvm::Instruction*>*) (/home/mahesha/ROCm/github/llvm/rel/build/bin/opt+0x182996d)
>  #6 0x00005565dc18ab6f llvm::convertConstantExprsToInstructions(llvm::Instruction*, llvm::ConstantExpr*, llvm::SmallPtrSetImpl<llvm::Instruction*>*) (/home/mahesha/ROCm/github/llvm/rel/build/bin/opt+0x182bb6f)
>  #7 0x00005565dcdca871 llvm::AMDGPU::replaceConstantUsesInFunction(llvm::ConstantExpr*, llvm::Function const*) (/home/mahesha/ROCm/github/llvm/rel/build/bin/opt+0x246b871)
>  #8 0x00005565db33af7f (anonymous namespace)::AMDGPULowerModuleLDS::processUsedLDS(llvm::Module&, llvm::Function*) AMDGPULowerModuleLDSPass.cpp:0:0
>  #9 0x00005565db33ee08 llvm::AMDGPULowerModuleLDSPass::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/home/mahesha/ROCm/github/llvm/rel/build/bin/opt+0x9dfe08)
> #10 0x00005565db0de9a1 llvm::detail::PassModel<llvm::Module, llvm::AMDGPULowerModuleLDSPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module> >::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/home/mahesha/ROCm/github/llvm/rel/build/bin/opt+0x77f9a1)
> #11 0x00005565dc172f54 llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module> >::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/home/mahesha/ROCm/github/llvm/rel/build/bin/opt+0x1813f54)
> #12 0x00005565db05f8c8 llvm::runPassPipeline(llvm::StringRef, llvm::Module&, llvm::TargetMachine*, llvm::TargetLibraryInfoImpl*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::StringRef, llvm::ArrayRef<llvm::StringRef>, llvm::opt_tool::OutputKind, llvm::opt_tool::VerifierKind, bool, bool, bool, bool, bool) (/home/mahesha/ROCm/github/llvm/rel/build/bin/opt+0x7008c8)
> #13 0x00005565dafdf821 main (/home/mahesha/ROCm/github/llvm/rel/build/bin/opt+0x680821)
> #14 0x00007f4cbd95fbf7 __libc_start_main /build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:344:0
> #15 0x00005565db05279a _start (/home/mahesha/ROCm/github/llvm/rel/build/bin/opt+0x6f379a)
> ```
I see. Thanks for checking!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112717/new/

https://reviews.llvm.org/D112717