[PATCH] D67133: [GlobalISel]: Fix some non determinism exposed in CSE due to not notifying observers about mutations + add verification for CSE

[Daniel Sanders via Phabricator via llvm-commits]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

LGTM with the changes Jessica and Quentin mentioned (or the XRay style `Error verify()` version).



================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/GISelChangeObserver.h:104
 
 /// A simple RAII based CSEInfo installer.
 /// Use this in a scope to install a delegate to the MachineFunction and reset
----------------
It predates this patch but the reference to CSEInfo here is inaccurate. That's only one usage of the delegate mechanism.


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2271-2272
 #endif // ifndef NDEBUG
     RAIIDelegateInstaller DelInstall(*MF, &WrapperObserver);
+    RAIIMFObserverInstaller ObsInstall(*MF, WrapperObserver);
     for (const BasicBlock *BB : RPOT) {
----------------
Is there ever a time when we want to do one of these and not the other? As far as I can see, installing the observer is always coupled with installing the delegate


================
Comment at: llvm/lib/CodeGen/GlobalISel/Legalizer.cpp:298-299
   }
+  if (CSEInfo)
+    CSEInfo->verify();
 
----------------
qcolombet wrote:
> aditya_nandakumar wrote:
> > paquette wrote:
> > > Should this be under `#ifndef NDEBUG?`
> > Definitely. Thanks for catching.
> I would rather have `verify` returns a bool and do:
> assert(!CSEInfo || CSEInfo->Info());
> I would rather have verify returns a bool and do:
> assert(!CSEInfo || CSEInfo->Info());

Offline, I commented to Aditya that the precedent was for verify() functions that return void and abort on failure but I do agree with this.

That said, I would like to point out that there's another possibility of returning an Error object like in XRay's BlockVerifier.h. That style has the benefit of being able to report exactly what's wrong and potentially annotate the state dump with indications as to which bit is invalid/stale.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67133/new/

https://reviews.llvm.org/D67133