[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:03 PST 2020
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/2170df76/attachment.html>
More information about the llvm-commits
mailing list