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

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

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?

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106881