[llvm] b91d9ec - [GlobalISel]: Fix some non determinism exposed in CSE due to not notifying observers about mutations + add verification for CSE
Vitaly Buka via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 19 13:00:34 PST 2020
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/38846
On Wed, 19 Feb 2020 at 13:00, Vitaly Buka <vitalybuka at google.com> wrote:
> This patch introduces memory bug:
>
> Testing: 0.. 10.. 20.. 30.. 40.
> FAIL: LLVM :: CodeGen/WebAssembly/PR40267.ll (17076 of 36117)
> ******************** TEST 'LLVM :: CodeGen/WebAssembly/PR40267.ll' FAILED ********************
> Script:
> --
> : 'RUN: at line 1'; /b/sanitizer-x86_64-linux-fast/build/llvm_build_asan/bin/llc < /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/test/CodeGen/WebAssembly/PR40267.ll -asm-verbose=false -verify-machineinstrs -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -wasm-keep-registers
> --
> Exit Code: 1
>
> Command Output (stderr):
> --
> =================================================================
> ==16208==ERROR: AddressSanitizer: use-after-poison on address 0x621000036798 at pc 0x000002d6b786 bp 0x7ffd1ab466b0 sp 0x7ffd1ab466a8
> READ of size 8 at 0x621000036798 thread T0
> #0 0x2d6b785 in operands_begin /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/CodeGen/MachineInstr.h:486:42
> #1 0x2d6b785 in defs /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/CodeGen/MachineInstr.h:515:23 #2 0x2d6b785 in (anonymous namespace)::WebAssemblyRegStackify::runOnMachineFunction(llvm::MachineFunction&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:926:37
> #3 0x4581dfd in llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/CodeGen/MachineFunctionPass.cpp:73:13
> #4 0x50ba4b0 in llvm::FPPassManager::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1482:27
> #5 0x50bac07 in llvm::FPPassManager::runOnModule(llvm::Module&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1518:16
> #6 0x50bba71 in runOnModule /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1583:27
> #7 0x50bba71 in llvm::legacy::PassManagerImpl::run(llvm::Module&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1695:44
> #8 0x9802a6 in compileModule(char**, llvm::LLVMContext&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/tools/llc/llc.cpp:617:8
> #9 0x979a1f in main /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/tools/llc/llc.cpp:356:22
> #10 0x7f1f109472e0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202e0)
> #11 0x8d20e9 in _start (/b/sanitizer-x86_64-linux-fast/build/llvm_build_asan/bin/llc+0x8d20e9)
>
> 0x621000036798 is located 2712 bytes inside of 4096-byte region [0x621000035d00,0x621000036d00)
> allocated by thread T0 here:
> #0 0x947cd4 in malloc /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
> #1 0xc1a70a in safe_malloc /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/Support/MemAlloc.h:26:18
> #2 0xc1a70a in Allocate /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/Support/AllocatorBase.h:85:12
> #3 0xc1a70a in llvm::BumpPtrAllocatorImpl<llvm::MallocAllocator, 4096ul, 4096ul, 128ul>::StartNewSlab() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/Support/Allocator.h:335:31
> #4 0xc1a485 in llvm::BumpPtrAllocatorImpl<llvm::MallocAllocator, 4096ul, 4096ul, 128ul>::Allocate(unsigned long, llvm::Align) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/Support/Allocator.h:188:5
> #5 0x455c6f3 in Allocate /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/Support/Allocator.h:202:12
> #6 0x455c6f3 in operator new<llvm::MallocAllocator, 4096, 4096, 128> /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/Support/Allocator.h:446:20
> #7 0x455c6f3 in llvm::MachineFunction::init() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/CodeGen/MachineFunction.cpp:158:15
> #8 0x45f1e12 in llvm::MachineModuleInfo::getOrCreateMachineFunction(llvm::Function const&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/CodeGen/MachineModuleInfo.cpp:250:14
> #9 0x4581c2d in llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/CodeGen/MachineFunctionPass.cpp:45:29
> #10 0x50ba4b0 in llvm::FPPassManager::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1482:27
> #11 0x50bac07 in llvm::FPPassManager::runOnModule(llvm::Module&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1518:16
> #12 0x50bba71 in runOnModule /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1583:27
> #13 0x50bba71 in llvm::legacy::PassManagerImpl::run(llvm::Module&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1695:44
> #14 0x9802a6 in compileModule(char**, llvm::LLVMContext&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/tools/llc/llc.cpp:617:8
> #15 0x979a1f in main /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/tools/llc/llc.cpp:356:22
> #16 0x7f1f109472e0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202e0)
>
>
> On Tue, 18 Feb 2020 at 14:55, Aditya Nandakumar via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>>
>> Author: Aditya Nandakumar
>> Date: 2020-02-18T14:54:57-08:00
>> New Revision: b91d9ec0bb8caedcdd1ddf0506fc19d6c55efae3
>>
>> URL:
>> https://github.com/llvm/llvm-project/commit/b91d9ec0bb8caedcdd1ddf0506fc19d6c55efae3
>> DIFF:
>> https://github.com/llvm/llvm-project/commit/b91d9ec0bb8caedcdd1ddf0506fc19d6c55efae3.diff
>>
>> LOG: [GlobalISel]: Fix some non determinism exposed in CSE due to not
>> notifying observers about mutations + add verification for CSE
>>
>> https://reviews.llvm.org/D67133
>>
>> While investigating some non determinism (CSE doesn't produce wrong
>> code, it just doesn't CSE some times) in GISel CSE on an out of tree
>> target, I realized that the core issue was that there were lots of code
>> that mutates (setReg, setRegClass etc), but doesn't notify observers
>> (CSE in this case but this could be any other observer). In order to
>> make the Observer be available in various parts of code and to avoid
>> having to thread it through various API, the MachineFunction now has the
>> observer as field. This allows it to be easily used in helper functions
>> such as constrainOperandRegClass.
>> Also added some invariant verification method in CSEInfo which can
>> catch these issues (when CSE is enabled).
>>
>> Added:
>>
>>
>> Modified:
>> llvm/include/llvm/CodeGen/GlobalISel/CSEInfo.h
>> llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
>> llvm/include/llvm/CodeGen/MachineFunction.h
>> llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp
>> llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp
>> llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
>> llvm/lib/CodeGen/GlobalISel/Legalizer.cpp
>> llvm/lib/CodeGen/GlobalISel/Utils.cpp
>>
>> Removed:
>>
>>
>>
>>
>> ################################################################################
>> diff --git a/llvm/include/llvm/CodeGen/GlobalISel/CSEInfo.h
>> b/llvm/include/llvm/CodeGen/GlobalISel/CSEInfo.h
>> index 13082e440d40..a3720eb35668 100644
>> --- a/llvm/include/llvm/CodeGen/GlobalISel/CSEInfo.h
>> +++ b/llvm/include/llvm/CodeGen/GlobalISel/CSEInfo.h
>> @@ -120,6 +120,8 @@ class GISelCSEInfo : public GISelChangeObserver {
>>
>> void setMF(MachineFunction &MF);
>>
>> + Error verify();
>> +
>> /// Records a newly created inst in a list and lazily insert it to the
>> CSEMap.
>> /// Sometimes, this method might be called with a partially constructed
>> /// MachineInstr,
>>
>> diff --git a/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
>> b/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
>> index e5691cb35174..d8fe4b3103db 100644
>> --- a/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
>> +++ b/llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h
>> @@ -101,7 +101,7 @@ class GISelObserverWrapper : public
>> MachineFunction::Delegate,
>> void MF_HandleRemoval(MachineInstr &MI) override { erasingInstr(MI); }
>> };
>>
>> -/// A simple RAII based CSEInfo installer.
>> +/// A simple RAII based Delegate installer.
>> /// Use this in a scope to install a delegate to the MachineFunction and
>> reset
>> /// it at the end of the scope.
>> class RAIIDelegateInstaller {
>> @@ -113,5 +113,27 @@ class RAIIDelegateInstaller {
>> ~RAIIDelegateInstaller();
>> };
>>
>> +/// A simple RAII based Observer installer.
>> +/// Use this in a scope to install the Observer to the MachineFunction
>> and reset
>> +/// it at the end of the scope.
>> +class RAIIMFObserverInstaller {
>> + MachineFunction &MF;
>> +
>> +public:
>> + RAIIMFObserverInstaller(MachineFunction &MF, GISelChangeObserver
>> &Observer);
>> + ~RAIIMFObserverInstaller();
>> +};
>> +
>> +/// Class to install both of the above.
>> +class RAIIMFObsDelInstaller {
>> + RAIIDelegateInstaller DelI;
>> + RAIIMFObserverInstaller ObsI;
>> +
>> +public:
>> + RAIIMFObsDelInstaller(MachineFunction &MF, GISelObserverWrapper
>> &Wrapper)
>> + : DelI(MF, &Wrapper), ObsI(MF, Wrapper) {}
>> + ~RAIIMFObsDelInstaller() = default;
>> +};
>> +
>> } // namespace llvm
>> #endif
>>
>> diff --git a/llvm/include/llvm/CodeGen/MachineFunction.h
>> b/llvm/include/llvm/CodeGen/MachineFunction.h
>> index 1ec1b4b2864f..171abf05d67e 100644
>> --- a/llvm/include/llvm/CodeGen/MachineFunction.h
>> +++ b/llvm/include/llvm/CodeGen/MachineFunction.h
>> @@ -72,6 +72,7 @@ class TargetRegisterClass;
>> class TargetSubtargetInfo;
>> struct WasmEHFuncInfo;
>> struct WinEHFuncInfo;
>> +class GISelChangeObserver;
>>
>> template <> struct ilist_alloc_traits<MachineBasicBlock> {
>> void deleteNode(MachineBasicBlock *MBB);
>> @@ -396,6 +397,7 @@ class MachineFunction {
>>
>> private:
>> Delegate *TheDelegate = nullptr;
>> + GISelChangeObserver *Observer = nullptr;
>>
>> using CallSiteInfoMap = DenseMap<const MachineInstr *, CallSiteInfo>;
>> /// Map a call instruction to call site arguments forwarding info.
>> @@ -444,6 +446,10 @@ class MachineFunction {
>> TheDelegate = delegate;
>> }
>>
>> + void setObserver(GISelChangeObserver *O) { Observer = O; }
>> +
>> + GISelChangeObserver *getObserver() const { return Observer; }
>> +
>> MachineModuleInfo &getMMI() const { return MMI; }
>> MCContext &getContext() const { return Ctx; }
>>
>>
>> diff --git a/llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp
>> b/llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp
>> index b12407a4a4f4..0e4fa219d988 100644
>> --- a/llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp
>> +++ b/llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp
>> @@ -261,6 +261,39 @@ void GISelCSEInfo::releaseMemory() {
>> #endif
>> }
>>
>> +Error GISelCSEInfo::verify() {
>> +#ifndef NDEBUG
>> + handleRecordedInsts();
>> + // For each instruction in map from MI -> UMI,
>> + // Profile(MI) and make sure UMI is found for that profile.
>> + for (auto &It : InstrMapping) {
>> + FoldingSetNodeID TmpID;
>> + GISelInstProfileBuilder(TmpID, *MRI).addNodeID(It.first);
>> + void *InsertPos;
>> + UniqueMachineInstr *FoundNode =
>> + CSEMap.FindNodeOrInsertPos(TmpID, InsertPos);
>> + if (FoundNode != It.second)
>> + return createStringError(std::errc::not_supported,
>> + "CSEMap mismatch, InstrMapping has MIs
>> without "
>> + "corresponding Nodes in CSEMap");
>> + }
>> +
>> + // For every node in the CSEMap, make sure that the InstrMapping
>> + // points to it.
>> + for (auto It = CSEMap.begin(), End = CSEMap.end(); It != End; ++It) {
>> + const UniqueMachineInstr &UMI = *It;
>> + if (!InstrMapping.count(UMI.MI))
>> + return createStringError(std::errc::not_supported,
>> + "Node in CSE without InstrMapping",
>> UMI.MI);
>> +
>> + if (InstrMapping[UMI.MI] != &UMI)
>> + return
>> createStringError(std::make_error_code(std::errc::not_supported),
>> + "Mismatch in CSE mapping");
>> + }
>> +#endif
>> + return Error::success();
>> +}
>> +
>> void GISelCSEInfo::print() {
>> LLVM_DEBUG(for (auto &It
>> : OpcodeHitTable) {
>> @@ -370,6 +403,7 @@ GISelCSEInfo &
>> GISelCSEAnalysisWrapper::get(std::unique_ptr<CSEConfigBase> CSEOpt,
>> bool Recompute) {
>> if (!AlreadyComputed || Recompute) {
>> + Info.releaseMemory();
>> Info.setCSEConfig(std::move(CSEOpt));
>> Info.analyze(*MF);
>> AlreadyComputed = true;
>>
>> diff --git a/llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp
>> b/llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp
>> index 62b903c30b89..bdaa6378e901 100644
>> --- a/llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp
>> +++ b/llvm/lib/CodeGen/GlobalISel/GISelChangeObserver.cpp
>> @@ -38,3 +38,11 @@
>> RAIIDelegateInstaller::RAIIDelegateInstaller(MachineFunction &MF,
>> }
>>
>> RAIIDelegateInstaller::~RAIIDelegateInstaller() {
>> MF.resetDelegate(Delegate); }
>> +
>> +RAIIMFObserverInstaller::RAIIMFObserverInstaller(MachineFunction &MF,
>> + GISelChangeObserver
>> &Observer)
>> + : MF(MF) {
>> + MF.setObserver(&Observer);
>> +}
>> +
>> +RAIIMFObserverInstaller::~RAIIMFObserverInstaller() {
>> MF.setObserver(nullptr); }
>>
>> diff --git a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
>> b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
>> index 418002003849..22a1eae3f480 100644
>> --- a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
>> +++ b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
>> @@ -2381,6 +2381,7 @@ bool
>> IRTranslator::runOnMachineFunction(MachineFunction &CurMF) {
>> WrapperObserver.addObserver(&Verifier);
>> #endif // ifndef NDEBUG
>> RAIIDelegateInstaller DelInstall(*MF, &WrapperObserver);
>> + RAIIMFObserverInstaller ObsInstall(*MF, WrapperObserver);
>> for (const BasicBlock *BB : RPOT) {
>> MachineBasicBlock &MBB = getMBB(*BB);
>> // Set the insertion point of all the following translations to
>>
>> diff --git a/llvm/lib/CodeGen/GlobalISel/Legalizer.cpp
>> b/llvm/lib/CodeGen/GlobalISel/Legalizer.cpp
>> index e789e4a333dc..0c0edc8a7b0c 100644
>> --- a/llvm/lib/CodeGen/GlobalISel/Legalizer.cpp
>> +++ b/llvm/lib/CodeGen/GlobalISel/Legalizer.cpp
>> @@ -28,6 +28,7 @@
>> #include "llvm/CodeGen/TargetSubtargetInfo.h"
>> #include "llvm/InitializePasses.h"
>> #include "llvm/Support/Debug.h"
>> +#include "llvm/Support/Error.h"
>> #include "llvm/Target/TargetMachine.h"
>>
>> #include <iterator>
>> @@ -180,7 +181,7 @@ Legalizer::legalizeMachineFunction(MachineFunction
>> &MF, const LegalizerInfo &LI,
>>
>> // Now install the observer as the delegate to MF.
>> // This will keep all the observers notified about new
>> insertions/deletions.
>> - RAIIDelegateInstaller DelInstall(MF, &WrapperObserver);
>> + RAIIMFObsDelInstaller Installer(MF, WrapperObserver);
>> LegalizerHelper Helper(MF, LI, WrapperObserver, MIRBuilder);
>> LegalizationArtifactCombiner ArtCombiner(MIRBuilder, MRI, LI);
>> auto RemoveDeadInstFromLists = [&WrapperObserver](MachineInstr
>> *DeadMI) {
>> @@ -305,6 +306,7 @@ bool Legalizer::runOnMachineFunction(MachineFunction
>> &MF) {
>> // We want CSEInfo in addition to WorkListObserver to observe all
>> changes.
>> AuxObservers.push_back(CSEInfo);
>> }
>> + assert(!CSEInfo || !errorToBool(CSEInfo->verify()));
>>
>> const LegalizerInfo &LI = *MF.getSubtarget().getLegalizerInfo();
>> MFResult Result = legalizeMachineFunction(MF, LI, AuxObservers,
>> *MIRBuilder);
>> @@ -324,5 +326,11 @@ bool Legalizer::runOnMachineFunction(MachineFunction
>> &MF) {
>> reportGISelFailure(MF, TPC, MORE, R);
>> return false;
>> }
>> + // If for some reason CSE was not enabled, make sure that we
>> invalidate the
>> + // CSEInfo object (as we currently declare that the analysis is
>> preserved).
>> + // The next time get on the wrapper is called, it will force it to
>> recompute
>> + // the analysis.
>> + if (!EnableCSE)
>> + Wrapper.setComputed(false);
>> return Result.Changed;
>> }
>>
>> diff --git a/llvm/lib/CodeGen/GlobalISel/Utils.cpp
>> b/llvm/lib/CodeGen/GlobalISel/Utils.cpp
>> index 73bc2a695965..32f866692835 100644
>> --- a/llvm/lib/CodeGen/GlobalISel/Utils.cpp
>> +++ b/llvm/lib/CodeGen/GlobalISel/Utils.cpp
>> @@ -12,6 +12,7 @@
>> #include "llvm/CodeGen/GlobalISel/Utils.h"
>> #include "llvm/ADT/APFloat.h"
>> #include "llvm/ADT/Twine.h"
>> +#include "llvm/CodeGen/GlobalISel/GISelChangeObserver.h"
>> #include "llvm/CodeGen/GlobalISel/RegisterBankInfo.h"
>> #include "llvm/CodeGen/MachineInstr.h"
>> #include "llvm/CodeGen/MachineInstrBuilder.h"
>> @@ -62,6 +63,15 @@ Register llvm::constrainOperandRegClass(
>> TII.get(TargetOpcode::COPY), Reg)
>> .addReg(ConstrainedReg);
>> }
>> + } else {
>> + if (GISelChangeObserver *Observer = MF.getObserver()) {
>> + if (!RegMO.isDef()) {
>> + MachineInstr *RegDef = MRI.getVRegDef(Reg);
>> + Observer->changedInstr(*RegDef);
>> + }
>> + Observer->changingAllUsesOfReg(MRI, Reg);
>> + Observer->finishedChangingAllUsesOfReg();
>> + }
>> }
>> return ConstrainedReg;
>> }
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200219/6113a9b3/attachment.html>
More information about the llvm-commits
mailing list