[PATCH] D106881: [WIP][GIlobalSel] Add GISelCSEAnalysisWrapperPass::verifyAnalysis

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar added a comment.

In D106881#2910097 <https://reviews.llvm.org/D106881#2910097>, @foad wrote:

> In D106881#2907835 <https://reviews.llvm.org/D106881#2907835>, @aditya_nandakumar wrote:
>
>> In D106881#2907544 <https://reviews.llvm.org/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();
  }




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106881