Skip to content

Commit

Permalink
Reapply "RegisterCoalescer: Add implicit-def of super register when c…
Browse files Browse the repository at this point in the history
…oalescing SUBREG_TO_REG"

This reverts commit e0f86ca.

This was hitting some assertions which have since been relaxed.
  • Loading branch information
arsenm committed Nov 7, 2023
1 parent d275277 commit ba385ae
Show file tree
Hide file tree
Showing 5 changed files with 668 additions and 55 deletions.
51 changes: 42 additions & 9 deletions llvm/lib/CodeGen/RegisterCoalescer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,11 @@ namespace {
/// number if it is not zero. If DstReg is a physical register and the
/// existing subregister number of the def / use being updated is not zero,
/// make sure to set it to the correct physical subregister.
void updateRegDefsUses(Register SrcReg, Register DstReg, unsigned SubIdx);
///
/// If \p IsSubregToReg, we are coalescing a DstReg = SUBREG_TO_REG
/// SrcReg. This introduces an implicit-def of DstReg on coalesced users.
void updateRegDefsUses(Register SrcReg, Register DstReg, unsigned SubIdx,
bool IsSubregToReg);

/// If the given machine operand reads only undefined lanes add an undef
/// flag.
Expand Down Expand Up @@ -1323,8 +1327,7 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
if (DstReg.isPhysical()) {
Register NewDstReg = DstReg;

unsigned NewDstIdx = TRI->composeSubRegIndices(CP.getSrcIdx(),
DefMI->getOperand(0).getSubReg());
unsigned NewDstIdx = TRI->composeSubRegIndices(CP.getSrcIdx(), DefSubIdx);
if (NewDstIdx)
NewDstReg = TRI->getSubReg(DstReg, NewDstIdx);

Expand Down Expand Up @@ -1473,7 +1476,7 @@ bool RegisterCoalescer::reMaterializeTrivialDef(const CoalescerPair &CP,
MRI->setRegClass(DstReg, NewRC);

// Update machine operands and add flags.
updateRegDefsUses(DstReg, DstReg, DstIdx);
updateRegDefsUses(DstReg, DstReg, DstIdx, false);
NewMI.getOperand(0).setSubReg(NewIdx);
// updateRegDefUses can add an "undef" flag to the definition, since
// it will replace DstReg with DstReg.DstIdx. If NewIdx is 0, make
Expand Down Expand Up @@ -1788,7 +1791,7 @@ void RegisterCoalescer::addUndefFlag(const LiveInterval &Int, SlotIndex UseIdx,
}

void RegisterCoalescer::updateRegDefsUses(Register SrcReg, Register DstReg,
unsigned SubIdx) {
unsigned SubIdx, bool IsSubregToReg) {
bool DstIsPhys = DstReg.isPhysical();
LiveInterval *DstInt = DstIsPhys ? nullptr : &LIS->getInterval(DstReg);

Expand Down Expand Up @@ -1828,16 +1831,22 @@ void RegisterCoalescer::updateRegDefsUses(Register SrcReg, Register DstReg,
if (DstInt && !Reads && SubIdx && !UseMI->isDebugInstr())
Reads = DstInt->liveAt(LIS->getInstructionIndex(*UseMI));

bool FullDef = true;

// Replace SrcReg with DstReg in all UseMI operands.
for (unsigned i = 0, e = Ops.size(); i != e; ++i) {
MachineOperand &MO = UseMI->getOperand(Ops[i]);

// Adjust <undef> flags in case of sub-register joins. We don't want to
// turn a full def into a read-modify-write sub-register def and vice
// versa.
if (SubIdx && MO.isDef())
if (SubIdx && MO.isDef()) {
MO.setIsUndef(!Reads);

if (!Reads)
FullDef = false;
}

// A subreg use of a partially undef (super) register may be a complete
// undef use now and then has to be marked that way.
if (MO.isUse() && !DstIsPhys) {
Expand Down Expand Up @@ -1869,6 +1878,25 @@ void RegisterCoalescer::updateRegDefsUses(Register SrcReg, Register DstReg,
MO.substVirtReg(DstReg, SubIdx, *TRI);
}

if (IsSubregToReg && !FullDef) {
// If the coalesed instruction doesn't fully define the register, we need
// to preserve the original super register liveness for SUBREG_TO_REG.
//
// We pretended SUBREG_TO_REG was a regular copy for coalescing purposes,
// but it introduces liveness for other subregisters. Downstream users may
// have been relying on those bits, so we need to ensure their liveness is
// captured with a def of other lanes.

// FIXME: Need to add new subrange if tracking subranges. We could also
// skip adding this if we knew the other lanes are dead, and only for
// other lanes.

assert(!MRI->shouldTrackSubRegLiveness(DstReg) &&
"this should update subranges");
MachineInstrBuilder MIB(*MF, UseMI);
MIB.addReg(DstReg, RegState::ImplicitDefine);
}

LLVM_DEBUG({
dbgs() << "\t\tupdated: ";
if (!UseMI->isDebugInstr())
Expand Down Expand Up @@ -2068,6 +2096,8 @@ bool RegisterCoalescer::joinCopy(MachineInstr *CopyMI, bool &Again) {
});
}

const bool IsSubregToReg = CopyMI->isSubregToReg();

ShrinkMask = LaneBitmask::getNone();
ShrinkMainRange = false;

Expand Down Expand Up @@ -2135,9 +2165,12 @@ bool RegisterCoalescer::joinCopy(MachineInstr *CopyMI, bool &Again) {

// Rewrite all SrcReg operands to DstReg.
// Also update DstReg operands to include DstIdx if it is set.
if (CP.getDstIdx())
updateRegDefsUses(CP.getDstReg(), CP.getDstReg(), CP.getDstIdx());
updateRegDefsUses(CP.getSrcReg(), CP.getDstReg(), CP.getSrcIdx());
if (CP.getDstIdx()) {
assert(!IsSubregToReg && "can this happen?");
updateRegDefsUses(CP.getDstReg(), CP.getDstReg(), CP.getDstIdx(), false);
}
updateRegDefsUses(CP.getSrcReg(), CP.getDstReg(), CP.getSrcIdx(),
IsSubregToReg);

// Shrink subregister ranges if necessary.
if (ShrinkMask.any()) {
Expand Down
Loading

3 comments on commit ba385ae

@hctim
Copy link
Collaborator

@hctim hctim commented on ba385ae Nov 7, 2023

Choose a reason for hiding this comment

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

Hey Matt, looks like this broke the MSan buildbot:

https://lab.llvm.org/buildbot/#/builders/74/builds/23298/steps/9/logs/stdio

[168/7664] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/APInt.cpp.o
FAILED: lib/Support/CMakeFiles/LLVMSupport.dir/APInt.cpp.o 
CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build_msan/lib/Support -I/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/Support -I/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build_msan/include -I/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/include -nostdinc++ -isystem /b/sanitizer-x86_64-linux-bootstrap-msan/build/libcxx_build_msan/include -isystem /b/sanitizer-x86_64-linux-bootstrap-msan/build/libcxx_build_msan/include/c++/v1 -fsanitize=memory -Wl,--rpath=/b/sanitizer-x86_64-linux-bootstrap-msan/build/libcxx_build_msan/lib -L/b/sanitizer-x86_64-linux-bootstrap-msan/build/libcxx_build_msan/lib -w -stdlib=libc++ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-class-memaccess -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fno-omit-frame-pointer -gline-tables-only -fsanitize=memory -fdiagnostics-color -ffunction-sections -fdata-sections -Werror=global-constructors -O3 -DNDEBUG -UNDEBUG -std=c++17  -fno-exceptions -funwind-tables -fno-rtti -MD -MT lib/Support/CMakeFiles/LLVMSupport.dir/APInt.cpp.o -MF lib/Support/CMakeFiles/LLVMSupport.dir/APInt.cpp.o.d -o lib/Support/CMakeFiles/LLVMSupport.dir/APInt.cpp.o -c /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/Support/APInt.cpp
clang++: /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/CodeGen/RegisterCoalescer.cpp:2792: JoinVals::ConflictResolution (anonymous namespace)::JoinVals::analyzeValue(unsigned int, (anonymous namespace)::JoinVals &): Assertion `(TrackSubRegLiveness || V.RedefVNI) && "Instruction is reading nonexistent value"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang++ -fsanitize=memory -L/b/sanitizer-x86_64-linux-bootstrap-msan/build/libcxx_build_msan/lib -w -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-class-memaccess -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fno-omit-frame-pointer -gline-tables-only -fsanitize=memory -fdiagnostics-color -ffunction-sections -fdata-sections -Werror=global-constructors -O3 -std=c++17 -fno-exceptions -funwind-tables -fno-rtti -Wl,--rpath=/b/sanitizer-x86_64-linux-bootstrap-msan/build/libcxx_build_msan/lib -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build_msan/lib/Support -I/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/Support -I/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build_msan/include -I/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/include -nostdinc++ -isystem /b/sanitizer-x86_64-linux-bootstrap-msan/build/libcxx_build_msan/include -isystem /b/sanitizer-x86_64-linux-bootstrap-msan/build/libcxx_build_msan/include/c++/v1 -stdlib=libc++ -DNDEBUG -UNDEBUG -c -MD -MT lib/Support/CMakeFiles/LLVMSupport.dir/APInt.cpp.o -MF lib/Support/CMakeFiles/LLVMSupport.dir/APInt.cpp.o.d -fcolor-diagnostics -o lib/Support/CMakeFiles/LLVMSupport.dir/APInt.cpp.o /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/Support/APInt.cpp
1.	<eof> parser at end of file
2.	Code generation
3.	Running pass 'Function Pass Manager' on module '/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/lib/Support/APInt.cpp'.
4.	Running pass 'Register Coalescer' on function '@_ZNK4llvm5APInt21multiplicativeInverseERKS0_'
 #0 0x0000556e729c6447 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x7f3d447)
 #1 0x0000556e729c404e llvm::sys::RunSignalHandlers() (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x7f3b04e)
 #2 0x0000556e7292d7f8 CrashRecoverySignalHandler(int) CrashRecoveryContext.cpp:0:0
 #3 0x00007f445ea3c460 (/lib/x86_64-linux-gnu/libc.so.6+0x3c460)
 #4 0x00007f445ea9152b pthread_kill (/lib/x86_64-linux-gnu/libc.so.6+0x9152b)
 #5 0x00007f445ea3c3b6 raise (/lib/x86_64-linux-gnu/libc.so.6+0x3c3b6)
 #6 0x00007f445ea2287c abort (/lib/x86_64-linux-gnu/libc.so.6+0x2287c)
 #7 0x00007f445ea2279b (/lib/x86_64-linux-gnu/libc.so.6+0x2279b)
 #8 0x00007f445ea33b66 (/lib/x86_64-linux-gnu/libc.so.6+0x33b66)
 #9 0x0000556e720b3069 (anonymous namespace)::JoinVals::computeAssignment(unsigned int, (anonymous namespace)::JoinVals&) RegisterCoalescer.cpp:0:0
#10 0x0000556e720aef4d (anonymous namespace)::JoinVals::mapValues((anonymous namespace)::JoinVals&) RegisterCoalescer.cpp:0:0
#11 0x0000556e720a6578 (anonymous namespace)::RegisterCoalescer::joinCopy(llvm::MachineInstr*, bool&) RegisterCoalescer.cpp:0:0
#12 0x0000556e720a47cd (anonymous namespace)::RegisterCoalescer::copyCoalesceWorkList(llvm::MutableArrayRef<llvm::MachineInstr*>) RegisterCoalescer.cpp:0:0
#13 0x0000556e720a2738 (anonymous namespace)::RegisterCoalescer::runOnMachineFunction(llvm::MachineFunction&) RegisterCoalescer.cpp:0:0
#14 0x0000556e71e9f37e llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x741637e)
#15 0x0000556e723ccc4b llvm::FPPassManager::runOnFunction(llvm::Function&) (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x7943c4b)
#16 0x0000556e723d5031 llvm::FPPassManager::runOnModule(llvm::Module&) (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x794c031)
#17 0x0000556e723cd6d5 llvm::legacy::PassManagerImpl::run(llvm::Module&) (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x79446d5)
#18 0x0000556e7316e9a3 clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::StringRef, llvm::Module*, clang::BackendAction, llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream>>) (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x86e59a3)
#19 0x0000556e73672629 clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) CodeGenAction.cpp:0:0
#20 0x0000556e74de7e24 clang::ParseAST(clang::Sema&, bool, bool) (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0xa35ee24)
#21 0x0000556e73584a90 clang::FrontendAction::Execute() (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x8afba90)
#22 0x0000556e734f36af clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x8a6a6af)
#23 0x0000556e7366ac16 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x8be1c16)
#24 0x0000556e70415602 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x598c602)
#25 0x0000556e70411a7d ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&, llvm::ToolContext const&) driver.cpp:0:0
#26 0x0000556e73352a19 void llvm::function_ref<void ()>::callback_fn<clang::driver::CC1Command::Execute(llvm::ArrayRef<std::optional<llvm::StringRef>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>*, bool*) const::$_1>(long) Job.cpp:0:0
#27 0x0000556e7292d53c llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x7ea453c)
#28 0x0000556e7335210f clang::driver::CC1Command::Execute(llvm::ArrayRef<std::optional<llvm::StringRef>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>*, bool*) const (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x88c910f)
#29 0x0000556e7330c4f9 clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, clang::driver::Command const*&, bool) const (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x88834f9)
#30 0x0000556e7330c7b7 clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*>>&, bool) const (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x88837b7)
#31 0x0000556e7332c887 clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*>>&) (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x88a3887)
#32 0x0000556e70410f17 clang_main(int, char**, llvm::ToolContext const&) (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x5987f17)
#33 0x0000556e704220d1 main (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x59990d1)
#34 0x00007f445ea23a90 (/lib/x86_64-linux-gnu/libc.so.6+0x23a90)
#35 0x00007f445ea23b49 __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x23b49)
#36 0x0000556e7040dee5 _start (/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin/clang+++0x5984ee5)
clang++: error: clang frontend command failed with exit code 134 (use -v to see invocation)
clang version 18.0.0
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build0/bin
clang++: note: diagnostic msg: 
********************
PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang++: note: diagnostic msg: /tmp/APInt-c826f3.cpp
clang++: note: diagnostic msg: /tmp/APInt-c826f3.sh
clang++: note: diagnostic msg: 
********************

I've reproduced it on my machine and verified the problem goes away (at main) with this patch reverted. I'll also revert 9832eb4 (the dependent tests) to make sure that check-all is green on main.

You can find out more info on how to reproduce the bot at https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild. In particular, the buildscript is buildbot_bootstrap_msan.sh. If you want to speed it up, comment out all the stuff with check_stage{2,3}_msan*. You can just leave the build_stage1_clang, check_stage1_msan, and build_stage2_msan to repro the problem.

Thanks,
Mitch.

@aemerson
Copy link
Contributor

Choose a reason for hiding this comment

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

This also broke the expensive checks bot on green dragon, which may be easier to repro: https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/26001/

@arsenm
Copy link
Contributor Author

@arsenm arsenm commented on ba385ae Nov 14, 2023

Choose a reason for hiding this comment

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

This also broke the expensive checks bot on green dragon, which may be easier to repro: https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/26001/

So far no luck trying to reproduce this. I'm quite surprised there are aarch64 failures here, I thought this barely did anything for not-x86. I haven't managed to hit either the x86 or aarch64 cases. This also seems to be using the non-runtimes compiler-rt build?

Please sign in to comment.