[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 14:06:04 PST 2020
My mistake, it's probably 52861809994c9199ceb45b98d982ab736a376e67.
On Wed, 19 Feb 2020 at 13:00, Vitaly Buka <vitalybuka at google.com> wrote:
> 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/68c6b97f/attachment.html>
More information about the llvm-commits
mailing list