This is an archive of the discontinued LLVM Phabricator instance.

[ScalarEvolution] Strictly enforce pointer/int type rules.
ClosedPublic

Authored by efriedma on Jul 6 2021, 1:27 PM.

Details

Summary

Rules:

  1. SCEVUnknown is a pointer if and only if the LLVM IR value is a pointer.
  2. SCEVPtrToInt is never a pointer.
  3. If any other SCEV expression has no pointer operands, the result is an integer.
  4. If a SCEVAddExpr has exactly one pointer operand, the result is a pointer.
  5. If a SCEVAddRecExpr's first operand is a pointer, and it has no other pointer operands, the result is a pointer.
  6. If a SCEVMinMaxExpr has all pointer operands, the result is a pointer.
  7. Otherwise, the SCEV expression is invalid.

I'm not sure how useful rule 6 is in practice. If we exclude it, we can guarantee that ScalarEvolution::getPointerBase always returns a SCEVUnknown, which might be a helpful property. Anyway, I'll leave that for a followup.

This is basically mop-up at this point; all the changes with significant functional effects have landed. Some of the remaining changes could be split off, but I don't see much point.

Diff Detail

Event Timeline

efriedma created this revision.Jul 6 2021, 1:27 PM
efriedma requested review of this revision.Jul 6 2021, 1:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2021, 1:27 PM
efriedma updated this revision to Diff 356818.Jul 6 2021, 1:56 PM

Fix an assertion failure I found through extra testing.

mkazantsev added inline comments.Jul 8 2021, 9:57 PM
llvm/lib/Analysis/ScalarEvolution.cpp
10600

It looks like it filters out such simple case as (LHS == RHS ) => (RHS== LHS) (if LHS = FoundRHS and RHS = FoundLHS). Could you please add a unit test to ensure it's not broken with this patch?

efriedma added inline comments.Jul 8 2021, 10:09 PM
llvm/lib/Analysis/ScalarEvolution.cpp
10600

This is inside the if (getTypeSizeInBits(LHS->getType()) < getTypeSizeInBits(FoundLHS->getType())). Since you can't extend/trunc a pointer, this means FoundLHS is not a pointer. I can't see how isImpliedCond could return any useful information in that case.

mkazantsev accepted this revision.Jul 9 2021, 3:17 AM

Looks fine!

llvm/lib/Analysis/ScalarEvolution.cpp
10600

Err, you are right.

This revision is now accepted and ready to land.Jul 9 2021, 3:17 AM
This revision was landed with ongoing or failed builds.Jul 9 2021, 5:33 PM
This revision was automatically updated to reflect the committed changes.

This caused assertions to be hit on some tests as well as real-world code that we run. Here is a testcase:

https://gist.github.com/kripken/a557a324095a96f3bc87f19bd3ed648f

That is the preprocessed output of test_atomic_cxx in emscripten. To see the crash, the build must have assertions enabled, and then run

clang++ a.cpp -target wasm32-unknown-unknown -c -O1

The crash is

clang++: llvm-project/llvm/lib/Analysis/ScalarEvolution.cpp:1888: const llvm::SCEV* llvm::ScalarEvolution::getSignExtendExpr(const llvm::SCEV*, llvm::Type*, unsigned int): Assertion `!Op->getType()->isPointerTy() && "Can't extend pointer!"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: build/bin/clang++ build/a.cpp -target wasm32-unknown-unknown -c -O1
1.      <eof> parser at end of file
2.      Optimizer
 #0 0x0000557c55c39ff0 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
 #1 0x0000557c55c37e54 llvm::sys::CleanupOnSignal(unsigned long) (build/bin/clang+++0x2184e54)
 #2 0x0000557c55b929a8 CrashRecoverySignalHandler(int) CrashRecoveryContext.cpp:0:0
 #3 0x00007f82f0f40140 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14140)
 #4 0x00007f82f0a23ce1 raise ./signal/../sysdeps/unix/sysv/linux/raise.c:51:1
 #5 0x00007f82f0a0d537 abort ./stdlib/abort.c:81:7
 #6 0x00007f82f0a0d40f get_sysdep_segment_value ./intl/loadmsgcat.c:509:8
 #7 0x00007f82f0a0d40f _nl_load_domain ./intl/loadmsgcat.c:970:34
 #8 0x00007f82f0a1c662 (/lib/x86_64-linux-gnu/libc.so.6+0x34662)
 #9 0x0000557c54d53d6f llvm::ScalarEvolution::getSignExtendExpr(llvm::SCEV const*, llvm::Type*, unsigned int) (build/bin/clang+++0x12a0d6f)
#10 0x0000557c55dbc194 WidenIV::getWideRecurrence(WidenIV::NarrowIVDefUse) (build/bin/clang+++0x2309194)
#11 0x0000557c55dc6ffc WidenIV::widenIVUse(WidenIV::NarrowIVDefUse, llvm::SCEVExpander&) (build/bin/clang+++0x2313ffc)
#12 0x0000557c55dc79d7 WidenIV::createWideIV(llvm::SCEVExpander&) (build/bin/clang+++0x23149d7)
#13 0x0000557c55dc8143 llvm::createWideIV(llvm::WideIVInfo const&, llvm::LoopInfo*, llvm::ScalarEvolution*, llvm::SCEVExpander&, llvm::DominatorTree*, llvm::SmallVectorImpl<llvm::WeakTrackingVH>&, unsigned int&, unsigned int&, bool, bool) (build/bin/clang+++0x2315143)
#14 0x0000557c559b5180 (anonymous namespace)::IndVarSimplify::run(llvm::Loop*) IndVarSimplify.cpp:0:0
#15 0x0000557c559b6eab llvm::IndVarSimplifyPass::run(llvm::Loop&, llvm::AnalysisManager<llvm::Loop, llvm::LoopStandardAnalysisResults&>&, llvm::LoopStandardAnalysisResults&, llvm::LPMUpdater&) (build/bin/clang+++0x1f03eab)
#16 0x0000557c56d51ebe llvm::detail::PassModel<llvm::Loop, llvm::IndVarSimplifyPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Loop, llvm::LoopStandardAnalysisResults&>, llvm::LoopStandardAnalysisResults&, llvm::LPMUpdater&>::run(llvm::Loop&, llvm::AnalysisManager<llvm::Loop, llvm::LoopStandardAnalysisResults&>&, llvm::LoopStandardAnalysisResults&, llvm::LPMUpdater&) (build/bin/clang+++0x329eebe)
#17 0x0000557c570687ca llvm::Optional<llvm::PreservedAnalyses> llvm::PassManager<llvm::Loop, llvm::AnalysisManager<llvm::Loop, llvm::LoopStandardAnalysisResults&>, llvm::LoopStandardAnalysisResults&, llvm::LPMUpdater&>::runSinglePass<llvm::Loop, std::unique_ptr<llvm::detail::PassConcept<llvm::Loop, llvm::AnalysisManager<llvm::Loop, llvm::LoopStandardAnalysisResults&>, llvm::LoopStandardAnalysisResults&, llvm::LPMUpdater&>, std::default_delete<llvm::detail::PassConcept<llvm::Loop, llvm::AnalysisManager<llvm::Loop, llvm::LoopStandardAnalysisResults&>, llvm::LoopStandardAnalysisResults&, llvm::LPMUpdater&> > > >(llvm::Loop&, std::unique_ptr<llvm::detail::PassConcept<llvm::Loop, llvm::AnalysisManager<llvm::Loop, llvm::LoopStandardAnalysisResults&>, llvm::LoopStandardAnalysisResults&, llvm::LPMUpdater&>, std::default_delete<llvm::detail::PassConcept<llvm::Loop, llvm::AnalysisManager<llvm::Loop, llvm::LoopStandardAnalysisResults&>, llvm::LoopStandardAnalysisResults&, llvm::LPMUpdater&> > >&, llvm::AnalysisManager<llvm::Loop, llvm::LoopStandardAnalysisResults&>&, llvm::LoopStandardAnalysisResults&, llvm::LPMUpdater&, llvm::PassInstrumentation&) (.isra.0) LoopPassManager.cpp:0:0
#18 0x0000557c57069d50 llvm::PassManager<llvm::Loop, llvm::AnalysisManager<llvm::Loop, llvm::LoopStandardAnalysisResults&>, llvm::LoopStandardAnalysisResults&, llvm::LPMUpdater&>::runWithoutLoopNestPasses(llvm::Loop&, llvm::AnalysisManager<llvm::Loop, llvm::LoopStandardAnalysisResults&>&, llvm::LoopStandardAnalysisResults&, llvm::LPMUpdater&) (build/bin/clang+++0x35b6d50)
#19 0x0000557c57069ef9 llvm::PassManager<llvm::Loop, llvm::AnalysisManager<llvm::Loop, llvm::LoopStandardAnalysisResults&>, llvm::LoopStandardAnalysisResults&, llvm::LPMUpdater&>::run(llvm::Loop&, llvm::AnalysisManager<llvm::Loop, llvm::LoopStandardAnalysisResults&>&, llvm::LoopStandardAnalysisResults&, llvm::LPMUpdater&) (build/bin/clang+++0x35b6ef9)
#20 0x0000557c5706bcf3 llvm::FunctionToLoopPassAdaptor::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (build/bin/clang+++0x35b8cf3)
#21 0x0000557c56d51b0e llvm::detail::PassModel<llvm::Function, llvm::FunctionToLoopPassAdaptor, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Function> >::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (build/bin/clang+++0x329eb0e)

This change also causes a crash on building SQLite (but that is much larger - I can provide it as well, however, if that would be useful).

This caused assertions to be hit on some tests as well as real-world code that we run.

Do all the failures you're seeing involve WidenIV::getWideRecurrence()? (If there are different callers of getSignExtendExpr triggering the same crash, I'll need to look at them separately.)

This caused assertions to be hit on some tests as well as real-world code that we run.

Do all the failures you're seeing involve WidenIV::getWideRecurrence()?

Yes, it looks like they all go through there.

Here are the full logs for -Os and -O3:

https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket.appspot.com/8841972864714597632/+/steps/Emscripten_testsuite__upstream__wasms_/0/stdout

https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket.appspot.com/8841972864714597632/+/steps/Emscripten_testsuite__upstream__wasm3_/0/stdout

All 8 crashes mention WidenIV::getWideRecurrence.

Crash should be fixed by rG6144085c29b3

Crash should be fixed by rG6144085c29b3

Thanks for the quick fix!

That looks right to me, but I'm not an LLVM expert.

uabelho added a subscriber: uabelho.EditedJul 13 2021, 1:20 AM

Hi,
I see one of the new asserts blow when SCEV is used from LoopReroll:

clang-13: ../lib/Analysis/ScalarEvolution.cpp:2983: const llvm::SCEV *llvm::ScalarEvolution::getMulExpr(SmallVectorImpl<const llvm::SCEV *> &, SCEV::NoWrapFlags, unsigned int): Assertion `!ETy->isPointerTy()' failed.
[...]
1.      <eof> parser at end of file
2.      Per-module optimization passes
3.      Running pass 'CallGraph Pass Manager' on module 'thvs3.c'.
4.      Running pass 'Loop Pass Manager' on function '@test'
5.      Running pass 'Reroll loops' on basic block '%for.body42.3.1'
 #0 0x0000000002d679e8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/repo/uabelho/llvm-project/llvm/build-all/bin/clang-13+0x2d679e8)
 #1 0x0000000002d6569e llvm::sys::RunSignalHandlers() (/repo/uabelho/llvm-project/llvm/build-all/bin/clang-13+0x2d6569e)
 #2 0x0000000002d68086 SignalHandler(int) Signals.cpp:0:0
 #3 0x00007fdf151ab630 __restore_rt sigaction.c:0:0
 #4 0x00007fdf128de387 raise (/lib64/libc.so.6+0x36387)
 #5 0x00007fdf128dfa78 abort (/lib64/libc.so.6+0x37a78)
 #6 0x00007fdf128d71a6 __assert_fail_base (/lib64/libc.so.6+0x2f1a6)
 #7 0x00007fdf128d7252 (/lib64/libc.so.6+0x2f252)
 #8 0x0000000001efa418 llvm::ScalarEvolution::getMulExpr(llvm::SmallVectorImpl<llvm::SCEV const*>&, llvm::SCEV::NoWrapFlags, unsigned int) (/repo/uabelho/llvm-project/llvm/build-all/bin/clang-13+0x1efa418)
 #9 0x0000000002b4f73f (anonymous namespace)::LoopReroll::DAGRootTracker::validateRootSet((anonymous namespace)::LoopReroll::DAGRootSet&) LoopRerollPass.cpp:0:0
#10 0x0000000002b4f288 (anonymous namespace)::LoopReroll::DAGRootTracker::findRootsBase(llvm::Instruction*, llvm::SmallPtrSet<llvm::Instruction*, 16u>) LoopRerollPass.cpp:0:0
#11 0x0000000002b4e51e (anonymous namespace)::LoopReroll::DAGRootTracker::findRootsRecursive(llvm::Instruction*, llvm::SmallPtrSet<llvm::Instruction*, 16u>) LoopRerollPass.cpp:0:0
#12 0x0000000002b4e694 (anonymous namespace)::LoopReroll::DAGRootTracker::findRootsRecursive(llvm::Instruction*, llvm::SmallPtrSet<llvm::Instruction*, 16u>) LoopRerollPass.cpp:0:0
#13 0x0000000002b4e694 (anonymous namespace)::LoopReroll::DAGRootTracker::findRootsRecursive(llvm::Instruction*, llvm::SmallPtrSet<llvm::Instruction*, 16u>) LoopRerollPass.cpp:0:0
#14 0x0000000002b46b71 (anonymous namespace)::LoopReroll::runOnLoop(llvm::Loop*) LoopRerollPass.cpp:0:0
#15 0x0000000002b491d2 (anonymous namespace)::LoopRerollLegacyPass::runOnLoop(llvm::Loop*, llvm::LPPassManager&) LoopRerollPass.cpp:0:0

Hi,
I see one of the new asserts blow when SCEV is used from LoopReroll:

clang-13: ../lib/Analysis/ScalarEvolution.cpp:2983: const llvm::SCEV *llvm::ScalarEvolution::getMulExpr(SmallVectorImpl<const llvm::SCEV *> &, SCEV::NoWrapFlags, unsigned int): Assertion `!ETy->isPointerTy()' failed.
[...]
1.      <eof> parser at end of file
2.      Per-module optimization passes
3.      Running pass 'CallGraph Pass Manager' on module 'thvs3.c'.
4.      Running pass 'Loop Pass Manager' on function '@test'
5.      Running pass 'Reroll loops' on basic block '%for.body42.3.1'
 #0 0x0000000002d679e8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/repo/uabelho/llvm-project/llvm/build-all/bin/clang-13+0x2d679e8)
 #1 0x0000000002d6569e llvm::sys::RunSignalHandlers() (/repo/uabelho/llvm-project/llvm/build-all/bin/clang-13+0x2d6569e)
 #2 0x0000000002d68086 SignalHandler(int) Signals.cpp:0:0
 #3 0x00007fdf151ab630 __restore_rt sigaction.c:0:0
 #4 0x00007fdf128de387 raise (/lib64/libc.so.6+0x36387)
 #5 0x00007fdf128dfa78 abort (/lib64/libc.so.6+0x37a78)
 #6 0x00007fdf128d71a6 __assert_fail_base (/lib64/libc.so.6+0x2f1a6)
 #7 0x00007fdf128d7252 (/lib64/libc.so.6+0x2f252)
 #8 0x0000000001efa418 llvm::ScalarEvolution::getMulExpr(llvm::SmallVectorImpl<llvm::SCEV const*>&, llvm::SCEV::NoWrapFlags, unsigned int) (/repo/uabelho/llvm-project/llvm/build-all/bin/clang-13+0x1efa418)
 #9 0x0000000002b4f73f (anonymous namespace)::LoopReroll::DAGRootTracker::validateRootSet((anonymous namespace)::LoopReroll::DAGRootSet&) LoopRerollPass.cpp:0:0
#10 0x0000000002b4f288 (anonymous namespace)::LoopReroll::DAGRootTracker::findRootsBase(llvm::Instruction*, llvm::SmallPtrSet<llvm::Instruction*, 16u>) LoopRerollPass.cpp:0:0
#11 0x0000000002b4e51e (anonymous namespace)::LoopReroll::DAGRootTracker::findRootsRecursive(llvm::Instruction*, llvm::SmallPtrSet<llvm::Instruction*, 16u>) LoopRerollPass.cpp:0:0
#12 0x0000000002b4e694 (anonymous namespace)::LoopReroll::DAGRootTracker::findRootsRecursive(llvm::Instruction*, llvm::SmallPtrSet<llvm::Instruction*, 16u>) LoopRerollPass.cpp:0:0
#13 0x0000000002b4e694 (anonymous namespace)::LoopReroll::DAGRootTracker::findRootsRecursive(llvm::Instruction*, llvm::SmallPtrSet<llvm::Instruction*, 16u>) LoopRerollPass.cpp:0:0
#14 0x0000000002b46b71 (anonymous namespace)::LoopReroll::runOnLoop(llvm::Loop*) LoopRerollPass.cpp:0:0
#15 0x0000000002b491d2 (anonymous namespace)::LoopRerollLegacyPass::runOnLoop(llvm::Loop*, llvm::LPPassManager&) LoopRerollPass.cpp:0:0

Reduced opt reproducer:

opt -loop-reroll loop-reroll.ll -S -o /dev/null

LoopReroll should be fixed by 5d1ba534

LoopReroll should be fixed by 5d1ba534

Yep, thanks!