Skip to content

[ValueLattice][SCCP] Do not track undefs #107105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Sep 3, 2024

SCCP currently has a lot of complexity related to undef tracking. The reason is that it wants to support undef v constant -> constant merges, which requires undef -> constant lattice transitions. This means that we cannot really do any evaluation if an input is undef, because it might become a constant later. So instead we leave those values as unknown. But of course that's not correct, because unknown means the value is poison. To resolve this we apply an iterative fixup stage where we replace those unknowns with overdefined instead.

This PR proposes to stop giving undef special treatment, and consider it like any other constant instead. This means that constant v undef now becomes overdefined and we may lose out on optimizations involving undefs in phis. On the flip side, we can remove a lot of complexity from SCCP. This PR still keeps constant_range_including_undef around to reduce scope, but we should remove that as well, further simplifying things.

I'd be interested in hearing some opinions on whether the undef tracking in SCCP still carries its weight or not...

@nikic nikic requested review from fhahn and dtcxzyw September 3, 2024 13:17
SCCP currently has a lot of complexity related to undef tracking.
The reason is that it wants to support undef v constant -> constant
merges, which requires undef -> constant lattice transitions. This
means that we cannot really do any evaluation if an input is undef,
because it might become a constant later. So instead we leave those
values as unknown. But of course that's not correct, because
unknown means the value is poison. As such, is an iterative fixup
stage where we replace those unknowns with overdefined instead.

This PR proposes to stop giving undef special treatment, and
consider it like any other constant instead. This means that
constant v undef now becomes overdefined and we may lose out
on optimizations involving undefs in phis. On the flip side, we
can remove a lot of complexity from SCCP. This PR still keeps
constant_range_including_undef around to reduce scope, but we
should remove that as well, further simplifying things.
@@ -35,6 +35,7 @@ typedef unsigned long long uint64_t;
// CHECK: while.cond.i:
// CHECK-NEXT: [[__TAGP_ADDR_0_I:%.*]] = phi ptr [ [[P:%.*]], [[ENTRY:%.*]] ], [ [[__TAGP_ADDR_1_I:%.*]], [[CLEANUP_I:%.*]] ]
// CHECK-NEXT: [[__R_0_I:%.*]] = phi i64 [ 0, [[ENTRY]] ], [ [[__R_1_I:%.*]], [[CLEANUP_I]] ]
// CHECK-NEXT: [[RETVAL_0_I:%.*]] = phi i64 [ undef, [[ENTRY]] ], [ [[RETVAL_1_I:%.*]], [[CLEANUP_I]] ]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test shows clear regressions. This is because we emit returns as stores to alloca and branch to common return block. For constant returns from blocks we can now end up with extra phi nodes due to the undef input introduced by mem2reg :(

Copy link

github-actions bot commented Sep 3, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 0797c184c636889f2897746dc71390ae28005c7c be08a5a9d2e9a56b3e9fa08b970aeec7ee3e2b0e --extensions cpp,h -- llvm/include/llvm/Analysis/ValueLattice.h llvm/include/llvm/Transforms/Utils/SCCPSolver.h llvm/lib/Analysis/ValueLattice.cpp llvm/lib/Transforms/IPO/FunctionSpecialization.cpp llvm/lib/Transforms/IPO/SCCP.cpp llvm/lib/Transforms/Scalar/SCCP.cpp llvm/lib/Transforms/Utils/SCCPSolver.cpp llvm/unittests/Analysis/ValueLatticeTest.cpp llvm/unittests/Transforms/IPO/FunctionSpecializationTest.cpp
View the diff from clang-format here.
diff --git a/llvm/lib/Transforms/Utils/SCCPSolver.cpp b/llvm/lib/Transforms/Utils/SCCPSolver.cpp
index 50f1e3f1e4..db14198b5b 100644
--- a/llvm/lib/Transforms/Utils/SCCPSolver.cpp
+++ b/llvm/lib/Transforms/Utils/SCCPSolver.cpp
@@ -814,9 +814,7 @@ public:
     handleCallResult(*Call);
   }
 
-  void resetInvalidated() {
-    Invalidated.clear();
-  }
+  void resetInvalidated() { Invalidated.clear(); }
 
   const ValueLatticeElement &getLatticeValueFor(Value *V) const {
     assert(!V->getType()->isStructTy() &&
@@ -2044,9 +2042,7 @@ void SCCPSolver::resetLatticeValueFor(CallBase *Call) {
   Visitor->resetLatticeValueFor(Call);
 }
 
-void SCCPSolver::resetInvalidated() {
-  Visitor->resetInvalidated();
-}
+void SCCPSolver::resetInvalidated() { Visitor->resetInvalidated(); }
 
 const ValueLatticeElement &SCCPSolver::getLatticeValueFor(Value *V) const {
   return Visitor->getLatticeValueFor(V);

dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Sep 3, 2024
Copy link
Member

@dtcxzyw dtcxzyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crash reproducer:

; bin/opt -passes=sccp reduced.ll -S

define void @debug_send_line1() {
  %1 = load ptr, ptr null, align 8
  %2 = load ptr, ptr %1, align 8
  call void %2()
  ret void
}

define void @debug_send_line2() {
  %1 = load ptr, ptr undef, align 8
  %2 = load ptr, ptr %1, align 8
  call void %2()
  ret void
}

define void @debug_send_line3() {
  %1 = load ptr, ptr undef, align 8
  %2 = load ptr, ptr %1, align 8
  call void %2()
  ret void
}
opt: /home/dtcxzyw/WorkSpace/Projects/compilers/llvm-project/llvm/lib/Transforms/Utils/SCCPSolver.cpp:826: const llvm::ValueLatticeElement& llvm::SCCPInstVisitor::getLatticeValueFor(llvm::Value*) const: Assertion `I != ValueState.end() && "V not found in ValueState nor Paramstate map!"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: bin/opt -passes=sccp reduced.ll -S
1.      Running pass "function(sccp)" on module "reduced.ll"
2.      Running pass "sccp" on function "debug_send_line1"
 #0 0x000077578b812c32 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/libLLVMSupport.so.20.0git+0x212c32)
 #1 0x000077578b80faff llvm::sys::RunSignalHandlers() (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/libLLVMSupport.so.20.0git+0x20faff)
 #2 0x000077578b80fc45 SignalHandler(int) Signals.cpp:0:0
 #3 0x000077578b242520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
 #4 0x000077578b2969fc __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #5 0x000077578b2969fc __pthread_kill_internal ./nptl/pthread_kill.c:78:10
 #6 0x000077578b2969fc pthread_kill ./nptl/pthread_kill.c:89:10
 #7 0x000077578b242476 gsignal ./signal/../sysdeps/posix/raise.c:27:6
 #8 0x000077578b2287f3 abort ./stdlib/abort.c:81:7
 #9 0x000077578b22871b _nl_load_domain ./intl/loadmsgcat.c:1177:9
#10 0x000077578b239e96 (/lib/x86_64-linux-gnu/libc.so.6+0x39e96)
#11 0x0000775784c5ba0d llvm::SCCPInstVisitor::getLatticeValueFor(llvm::Value*) const (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/../lib/libLLVMTransformUtils.so.20.0git+0x25ba0d)
#12 0x0000775784c6bfc8 llvm::SCCPInstVisitor::getConstantOrNull(llvm::Value*) const (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/../lib/libLLVMTransformUtils.so.20.0git+0x26bfc8)
#13 0x0000775784c6c630 llvm::SCCPSolver::tryToReplaceWithConstant(llvm::Value*) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/../lib/libLLVMTransformUtils.so.20.0git+0x26c630)
#14 0x0000775784c6cc08 llvm::SCCPSolver::simplifyInstsInBlock(llvm::BasicBlock&, llvm::SmallPtrSetImpl<llvm::Value*>&, llvm::TrackingStatistic&, llvm::TrackingStatistic&) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/../lib/libLLVMTransformUtils.so.20.0git+0x26cc08)
#15 0x000077578533fa02 llvm::SCCPPass::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/../lib/libLLVMScalarOpts.so.20.0git+0x33fa02)
#16 0x0000775786cd4145 llvm::detail::PassModel<llvm::Function, llvm::SCCPPass, llvm::AnalysisManager<llvm::Function>>::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/../lib/libLLVMPasses.so.20.0git+0xd4145)
#17 0x0000775783f51d0d llvm::PassManager<llvm::Function, llvm::AnalysisManager<llvm::Function>>::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/../lib/libLLVMCore.so.20.0git+0x351d0d)
#18 0x000077578a2d7f95 llvm::detail::PassModel<llvm::Function, llvm::PassManager<llvm::Function, llvm::AnalysisManager<llvm::Function>>, llvm::AnalysisManager<llvm::Function>>::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/../lib/libLLVMX86CodeGen.so.20.0git+0xd7f95)
#19 0x0000775783f50876 llvm::ModuleToFunctionPassAdaptor::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/../lib/libLLVMCore.so.20.0git+0x350876)
#20 0x000077578a2d8955 llvm::detail::PassModel<llvm::Module, llvm::ModuleToFunctionPassAdaptor, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/../lib/libLLVMX86CodeGen.so.20.0git+0xd8955)
#21 0x0000775783f4e83a llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/../lib/libLLVMCore.so.20.0git+0x34e83a)
#22 0x000077578b972fc7 llvm::runPassPipeline(llvm::StringRef, llvm::Module&, llvm::TargetMachine*, llvm::TargetLibraryInfoImpl*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::StringRef, llvm::ArrayRef<llvm::PassPlugin>, llvm::ArrayRef<std::function<void (llvm::PassBuilder&)>>, llvm::opt_tool::OutputKind, llvm::opt_tool::VerifierKind, bool, bool, bool, bool, bool, bool, bool) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/libLLVMOptDriver.so.20.0git+0x2cfc7)
#23 0x000077578b97e962 optMain (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/libLLVMOptDriver.so.20.0git+0x38962)
#24 0x000077578b229d90 __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#25 0x000077578b229e40 call_init ./csu/../csu/libc-start.c:128:20
#26 0x000077578b229e40 __libc_start_main ./csu/../csu/libc-start.c:379:5
#27 0x000063ae8b28e095 _start (bin/opt+0x1095)
Aborted (core dumped)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants