[WIP][GIlobalSel] Add GISelCSEAnalysisWrapperPass::verifyAnalysis

[foad]

Summary

This enables automatic pass manager verification of the CSE analysis
after every pass that claims to preserve it.

TODO: avoid the ugly const_cast by rewriting GISelCSEInfo::verify() to
work in a const-friendly way?

TODO: fix the AMDGPU lit test failures

[foad]

Any thoughts on this? I thought it would be cleaner to use the standard analysis verification API instead of ad hoc calls to CSEInfo->verify from the Legalizer and Combiner.

[aditya_nandakumar]

In D106881#2907544, @foad wrote:

Any thoughts on this? I thought it would be cleaner to use the standard analysis verification API instead of ad hoc calls to CSEInfo->verify from the Legalizer and Combiner.

It does appear cleaner once we make sure that all backends are clean wrt reporting all mutations to the observers (last time I checked there were failures and I didn't get a chance to verify and fix). If that's indeed the case, then LGTM.

[foad]

In D106881#2907835, @aditya_nandakumar wrote:

In D106881#2907544, @foad wrote:

Any thoughts on this? I thought it would be cleaner to use the standard analysis verification API instead of ad hoc calls to CSEInfo->verify from the Legalizer and Combiner.

It does appear cleaner once we make sure that all backends are clean wrt reporting all mutations to the observers (last time I checked there were failures and I didn't get a chance to verify and fix). If that's indeed the case, then LGTM.

There are still some AMDGPU lit test failures, because we do some things in the legalizer that are not safe wrt observers.

On the other hand, the AMDGPU legalizer doesn't use CSEMIRBuilder, so is there any need for it to report stuff to the observers?

On the other other hand, the AMDGPU legalizer probably should use CSEMIRBuilder. The main problem I saw last time I tried this was with MRI->setRegClass(Reg, RC). As I understand it, this counts as a mutation of not just the instruction that defines Reg, but every instruction that uses Reg too. Is that correct? Is there a way to report that kind of widespread mutation to the observers?

[aditya_nandakumar]

In D106881#2910097, @foad wrote:

In D106881#2907835, @aditya_nandakumar wrote:

In D106881#2907544, @foad wrote:

Any thoughts on this? I thought it would be cleaner to use the standard analysis verification API instead of ad hoc calls to CSEInfo->verify from the Legalizer and Combiner.

It does appear cleaner once we make sure that all backends are clean wrt reporting all mutations to the observers (last time I checked there were failures and I didn't get a chance to verify and fix). If that's indeed the case, then LGTM.

There are still some AMDGPU lit test failures, because we do some things in the legalizer that are not safe wrt observers.

On the other hand, the AMDGPU legalizer doesn't use CSEMIRBuilder, so is there any need for it to report stuff to the observers?

If CSE is not enabled, then I think the verifier should not complain (CSEs). The main contract is to notify Observers of mutations (technically not necessary if there's nothing observing, but still better to notify).

On the other other hand, the AMDGPU legalizer probably should use CSEMIRBuilder. The main problem I saw last time I tried this was with MRI->setRegClass(Reg, RC). As I understand it, this counts as a mutation of not just the instruction that defines Reg, but every instruction that uses Reg too. Is that correct? Is there a way to report that kind of widespread mutation to the observers?

Yes there is.

if (GISelChangeObserver *Observer = MF.getObserver()) {
  Register Reg = ...;
  Observer->changingAllUsesOfReg(MRI, Reg);
  // change it.
  Observer->finishedChangingAllUsesOfReg();
}

llvm/include/llvm/CodeGen/GlobalISel/CSEInfo.h

public:
   /// If CSEConfig is already set, and the CSE Analysis has been preserved,
   /// it will not use the new CSEOpt(use Recompute to force using the new
   /// CSEOpt).
   GISelCSEInfo &get(std::unique_ptr<CSEConfigBase> CSEOpt,
                     bool ReCompute = false);
   void setMF(MachineFunction &MFunc) { MF = &MFunc; }
   void setComputed(bool Computed) { AlreadyComputed = Computed; }
   void releaseMemory() { Info.releaseMemory(); }
+  void verify();
 };
 
 /// The actual analysis pass wrapper.
 class GISelCSEAnalysisWrapperPass : public MachineFunctionPass {
   GISelCSEAnalysisWrapper Wrapper;
 
 public:
   static char ID;
   GISelCSEAnalysisWrapperPass();
 
   void getAnalysisUsage(AnalysisUsage &AU) const override;
 
   const GISelCSEAnalysisWrapper &getCSEWrapper() const { return Wrapper; }
   GISelCSEAnalysisWrapper &getCSEWrapper() { return Wrapper; }
 
   bool runOnMachineFunction(MachineFunction &MF) override;
 
   void releaseMemory() override {
     Wrapper.releaseMemory();
     Wrapper.setComputed(false);
   }
+
+  void verifyAnalysis() const override {
+    const_cast<GISelCSEAnalysisWrapper *>(&Wrapper)->verify();
+  }
 };
 
 } // namespace llvm
 
 #endif

llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp

GISelCSEAnalysisWrapper::get(std::unique_ptr<CSEConfigBase> CSEOpt,
   if (!AlreadyComputed || Recompute) {
     Info.releaseMemory();
     Info.setCSEConfig(std::move(CSEOpt));
     Info.analyze(*MF);
     AlreadyComputed = true;
   }
   return Info;
 }
+
+void GISelCSEAnalysisWrapper::verify() { cantFail(Info.verify()); }
+
 void GISelCSEAnalysisWrapperPass::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.setPreservesAll();
   MachineFunctionPass::getAnalysisUsage(AU);
 }
 
 bool GISelCSEAnalysisWrapperPass::runOnMachineFunction(MachineFunction &MF) {
   releaseMemory();
   Wrapper.setMF(MF);
   return false;
 }