TargetMachine: Indicate whether machine verifier passes.

[MatzeB]

Summary

This adds a callback to the LLVMTargetMachine that lets target indicate
that they do not pass the machine verifier checks in all cases yet.

This is intended to be a temporary measure while the targets are fixed
allowing us to enable the machine verifier by default in expensive
checks settings!

[pftbest]

MSP430 was fixed, it should pass the tests now.

[MatzeB]

MSP430 was fixed, it should pass the tests now.

Great, I removed the overload!

[jlebar]

lgtm for NVPTX changes.

[fhahn]

Looks good, thanks for picking that up. Unfortunately I did not have any time to work on the machine verification pass recently.

[jpienaar]

Looks good for Lanai changes

[sdardis]

LGTM for MIPS changes.

[tstellar]

lib/Target/AMDGPU/AMDGPUTargetMachine.h

73–75 ↗

(On Diff #100784)

I think GCNTargetMachine should be able to return true for this, did you try this?

[filcab]

Should we add TODO comments to all those overrides and state that the errors should be fixed and the overrides (and the function itself, afterwards) be removed?

[RKSimon]

x86 looks good.

Probably the most pragmatic solution for now, I'm just concerned that this will lead to some targets never getting fixed......

[MatzeB]

In D33696#768678, @RKSimon wrote:

x86 looks good.

Probably the most pragmatic solution for now, I'm just concerned that this will lead to some targets never getting fixed......

Indeed I also have some worries that that callback will never go away once introduced. However I find the status quo where pretty much all targets are broken even more worrying and I am optimistic that this function acts more as a motivation and goal when people see that other targets are clean and theirs isn't.

In D33696#768657, @filcab wrote:

Should we add TODO comments to all those overrides and state that the errors should be fixed and the overrides (and the function itself, afterwards) be removed?

I will add a comment that the function is a stopgap measure and will be removed. My plan for the targets is to file PRs against them which I hope works better than TODOs in the source.

lib/Target/AMDGPU/AMDGPUTargetMachine.h

73–75 ↗

(On Diff #100784)

No I have not tested the individual AMDGPU target machines, I'll try.

[RKSimon]

include/llvm/Target/TargetMachine.h

305 ↗

(On Diff #100784)

Would it be too strict to add a comment saying that targets should not become 'unclean' again once they are passing expensive checks?

lib/Target/AVR/AVRTargetMachine.h

46 ↗

(On Diff #100784)

FYI I'm not sure we have any EXPENSIVE_CHECKS buildbot that enables AVR (or WebAssembly).

lib/Target/Hexagon/HexagonTargetMachine.h

49 ↗

(On Diff #100784)

Hexagon was clean on my last set of tests (PR32146 / PR33048)

[dylanmckay]

lib/Target/AVR/AVRTargetMachine.h

46 ↗

(On Diff #100784)

Can confirm in the AVR case.

[MatzeB]

lib/Target/AVR/AVRTargetMachine.h

46 ↗

(On Diff #100784)

No I don't think we have. I also added this return false; defensively feel free to remove it if AVR is clean.

BTW: You can always manually run all lit tests with verify-machineinstrs with llvm-lit -Dllc="llc -verify-machineinstrs" that saves you the time to recompile with EXPENSIVE_CHECKS enabled.

llvm/trunk/include/llvm/Target/TargetMachine.h

public:
 
   /// Add passes to the specified pass manager to get machine code emitted with
   /// the MCJIT. This method returns true if machine code is not supported. It
   /// fills the MCContext Ctx pointer which can be used to build custom
   /// MCStreamer.
   bool addPassesToEmitMC(PassManagerBase &PM, MCContext *&Ctx,
                          raw_pwrite_stream &OS,
                          bool DisableVerify = true) override;
+
+  /// Returns true if the target is expected to pass all machine verifier
+  /// checks. This is a stopgap measure to fix targets one by one. We will
+  /// remove this at some point and always enable the verifier when
+  /// EXPENSIVE_CHECKS is enabled.
+  virtual bool isMachineVerifierClean() const { return true; }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_TARGET_TARGETMACHINE_H

llvm/trunk/lib/CodeGen/TargetPassConfig.cpp

 }
 
 void TargetPassConfig::addPrintPass(const std::string &Banner) {
   if (TM->shouldPrintMachineCode())
     PM->add(createMachineFunctionPrinterPass(dbgs(), Banner));
 }
 
 void TargetPassConfig::addVerifyPass(const std::string &Banner) {
-  if (VerifyMachineCode)
+  bool Verify = VerifyMachineCode;
+#ifdef EXPENSIVE_CHECKS
+  if (VerifyMachineCode == cl::BOU_UNSET)
+    Verify = TM->isMachineVerifierClean();
+#endif
+  if (Verify)
     PM->add(createMachineVerifierPass(Banner));
 }
 
 /// Add common target configurable passes that perform LLVM IR to IR transforms
 /// following machine independent optimization.
 void TargetPassConfig::addIRPasses() {
   switch (UseCFLAA) {
   case CFLAAType::Steensgaard:

llvm/trunk/lib/Target/AMDGPU/AMDGPUTargetMachine.h

public:
 
   void adjustPassManager(PassManagerBuilder &) override;
   /// Get the integer value of a null pointer in the given address space.
   uint64_t getNullPointerValue(unsigned AddrSpace) const {
     if (AddrSpace == AS.LOCAL_ADDRESS || AddrSpace == AS.REGION_ADDRESS)
       return -1;
     return 0;
   }
-
 };
 
 //===----------------------------------------------------------------------===//
 // R600 Target Machine (R600 -> Cayman)
 //===----------------------------------------------------------------------===//
 
 class R600TargetMachine final : public AMDGPUTargetMachine {
 private:
   mutable StringMap<std::unique_ptr<R600Subtarget>> SubtargetMap;
 
 public:
   R600TargetMachine(const Target &T, const Triple &TT, StringRef CPU,
                     StringRef FS, TargetOptions Options,
                     Optional<Reloc::Model> RM, CodeModel::Model CM,
                     CodeGenOpt::Level OL);
 
   TargetPassConfig *createPassConfig(PassManagerBase &PM) override;
 
   const R600Subtarget *getSubtargetImpl(const Function &) const override;
+
+  bool isMachineVerifierClean() const override {
+    return false;
+  }
 };
 
 //===----------------------------------------------------------------------===//
 // GCN Target Machine (SI+)
 //===----------------------------------------------------------------------===//
 
 class GCNTargetMachine final : public AMDGPUTargetMachine {
 private:

llvm/trunk/lib/Target/ARM/ARMTargetMachine.h

public:
   TargetIRAnalysis getTargetIRAnalysis() override;
 
   // Pass Pipeline Configuration
   TargetPassConfig *createPassConfig(PassManagerBase &PM) override;
 
   TargetLoweringObjectFile *getObjFileLowering() const override {
     return TLOF.get();
   }
+
+  bool isMachineVerifierClean() const override {
+    return false;
+  }
 };
 
 /// ARM/Thumb little endian target machine.
 ///
 class ARMLETargetMachine : public ARMBaseTargetMachine {
 public:
   ARMLETargetMachine(const Target &T, const Triple &TT, StringRef CPU,
                      StringRef FS, const TargetOptions &Options,

llvm/trunk/lib/Target/AVR/AVRTargetMachine.h

public:
   const AVRSubtarget *getSubtargetImpl(const Function &) const override;
 
   TargetLoweringObjectFile *getObjFileLowering() const override {
     return this->TLOF.get();
   }
 
   TargetPassConfig *createPassConfig(PassManagerBase &PM) override;
 
+  bool isMachineVerifierClean() const override {
+    return false;
+  }
+
 private:
   std::unique_ptr<TargetLoweringObjectFile> TLOF;
   AVRSubtarget SubTarget;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_AVR_TARGET_MACHINE_H

llvm/trunk/lib/Target/Lanai/LanaiTargetMachine.h

public:
   TargetIRAnalysis getTargetIRAnalysis() override;
 
   // Pass Pipeline Configuration
   TargetPassConfig *createPassConfig(PassManagerBase &pass_manager) override;
 
   TargetLoweringObjectFile *getObjFileLowering() const override {
     return TLOF.get();
   }
+
+  bool isMachineVerifierClean() const override {
+    return false;
+  }
 };
 } // namespace llvm
 
 #endif // LLVM_LIB_TARGET_LANAI_LANAITARGETMACHINE_H

llvm/trunk/lib/Target/Mips/MipsTargetMachine.h

public:
   TargetPassConfig *createPassConfig(PassManagerBase &PM) override;
 
   TargetLoweringObjectFile *getObjFileLowering() const override {
     return TLOF.get();
   }
 
   bool isLittleEndian() const { return isLittle; }
   const MipsABIInfo &getABI() const { return ABI; }
+
+  bool isMachineVerifierClean() const override {
+    return false;
+  }
 };
 
 /// Mips32/64 big endian target machine.
 ///
 class MipsebTargetMachine : public MipsTargetMachine {
   virtual void anchor();
 
 public:

llvm/trunk/lib/Target/NVPTX/NVPTXTargetMachine.h

public:
   TargetLoweringObjectFile *getObjFileLowering() const override {
     return TLOF.get();
   }
 
   void adjustPassManager(PassManagerBuilder &) override;
 
   TargetIRAnalysis getTargetIRAnalysis() override;
 
+  bool isMachineVerifierClean() const override {
+    return false;
+  }
 }; // NVPTXTargetMachine.
 
 class NVPTXTargetMachine32 : public NVPTXTargetMachine {
   virtual void anchor();
 public:
   NVPTXTargetMachine32(const Target &T, const Triple &TT, StringRef CPU,
                        StringRef FS, const TargetOptions &Options,
                        Optional<Reloc::Model> RM, CodeModel::Model CM,

llvm/trunk/lib/Target/PowerPC/PPCTargetMachine.h

public:
   TargetLoweringObjectFile *getObjFileLowering() const override {
     return TLOF.get();
   }
   bool isELFv2ABI() const { return TargetABI == PPC_ABI_ELFv2; }
   bool isPPC64() const {
     const Triple &TT = getTargetTriple();
     return (TT.getArch() == Triple::ppc64 || TT.getArch() == Triple::ppc64le);
   };
+
+  bool isMachineVerifierClean() const override {
+    return false;
+  }
 };
 
 /// PowerPC 32-bit target machine.
 ///
 class PPC32TargetMachine : public PPCTargetMachine {
   virtual void anchor();
 public:
   PPC32TargetMachine(const Target &T, const Triple &TT, StringRef CPU,

llvm/trunk/lib/Target/Sparc/SparcTargetMachine.h

public:
   const SparcSubtarget *getSubtargetImpl() const { return &Subtarget; }
   const SparcSubtarget *getSubtargetImpl(const Function &) const override;
 
   // Pass Pipeline Configuration
   TargetPassConfig *createPassConfig(PassManagerBase &PM) override;
   TargetLoweringObjectFile *getObjFileLowering() const override {
     return TLOF.get();
   }
+
+  bool isMachineVerifierClean() const override {
+    return false;
+  }
 };
 
 /// Sparc 32-bit target machine
 ///
 class SparcV8TargetMachine : public SparcTargetMachine {
   virtual void anchor();
 public:
   SparcV8TargetMachine(const Target &T, const Triple &TT, StringRef CPU,

llvm/trunk/lib/Target/SystemZ/SystemZTargetMachine.h

public:
   TargetPassConfig *createPassConfig(PassManagerBase &PM) override;
   TargetIRAnalysis getTargetIRAnalysis() override;
 
   TargetLoweringObjectFile *getObjFileLowering() const override {
     return TLOF.get();
   }
 
   bool targetSchedulesPostRAScheduling() const override { return true; };
+
+  bool isMachineVerifierClean() const override { return false; }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_SYSTEMZ_SYSTEMZTARGETMACHINE_H

llvm/trunk/lib/Target/X86/X86TargetMachine.h

public:
   TargetIRAnalysis getTargetIRAnalysis() override;
 
   // Set up the pass pipeline.
   TargetPassConfig *createPassConfig(PassManagerBase &PM) override;
 
   TargetLoweringObjectFile *getObjFileLowering() const override {
     return TLOF.get();
   }
+
+  bool isMachineVerifierClean() const override {
+    return false;
+  }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_X86_X86TARGETMACHINE_H

llvm/trunk/test/CodeGen/MIR/Generic/multiRunPass.mir

 # RUN: llc -run-pass expand-isel-pseudos  -run-pass peephole-opt -debug-pass=Arguments -o - %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=PSEUDO_PEEPHOLE
 # RUN: llc -run-pass expand-isel-pseudos,peephole-opt -debug-pass=Arguments -o - %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=PSEUDO_PEEPHOLE
 # RUN: llc -run-pass peephole-opt -run-pass expand-isel-pseudos -debug-pass=Arguments -o - %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=PEEPHOLE_PSEUDO
 # RUN: llc -run-pass peephole-opt,expand-isel-pseudos -debug-pass=Arguments -o - %s 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=PEEPHOLE_PSEUDO
 # REQUIRES: asserts
 
 # This test ensures that the command line accepts
 # several run passes on the same command line and
 # actually create the proper pipeline for it.
-# PSEUDO_PEEPHOLE: -expand-isel-pseudos -peephole-opt
-# PEEPHOLE_PSEUDO: -peephole-opt -expand-isel-pseudos
+# PSEUDO_PEEPHOLE: -expand-isel-pseudos {{(-machineverifier )?}}-peephole-opt
+# PEEPHOLE_PSEUDO: -peephole-opt {{(-machineverifier )?}}-expand-isel-pseudos
 
 # Make sure there are no other passes happening after what we asked.
 # CHECK-NEXT: --- |
 ---
 # CHECK: name: foo
 name: foo
 body: |
   bb.0:
 ...