[PATCH] D111397: Add new MachineFunction property FailsVerification

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MIRYamlMapping.h:726
     YamlIO.mapOptional("hasWinCFI", MF.HasWinCFI, false);
+    YamlIO.mapOptional("failsVerification", MF.FailsVerification, false);
     YamlIO.mapOptional("registers", MF.VirtualRegisters,
----------------
arsenm wrote:
> foad wrote:
> > arsenm wrote:
> > > This should not need explicit serialization. The verifier runs after mir parsing anyway. 
> > > 
> > > It should also be illegal to clear this after it’s set 
> > > This should not need explicit serialization.
> > 
> > Why? It means "this function has known problems that I haven't fixed yet so please don't run the verifier on it".
> > 
> > > It should also be illegal to clear this after it’s set
> > 
> > Why?
> Oh, I misread the intent of this change. I assumed this was solving a problem I've run into where a pass introduces a compilation failure, and as a result ends up in some invalid state where the verifier crashes. For example this happens when the register allocator fails and emits an error, but the compilation still needs to continue.
> 
> I don't love the idea of allowing functions to fail the verifier, and then be fixed up later. Once the verifier fails a function should be dead. 
Yeah, `FailsVerifier` is an intentional decision by pass authors, that they are aware that the pass produces MIR that will not pass the verifier.

I am pretty sure everyone would be happier to just force every machine function to very, but it's a practical concern to get to that state. Before this change we had verification completely disable for a number of passes, with this change at least we can enable verification for all passes in general and the instances where this isn't the case target authors can work around by setting the `FailsVerification` flag and fixing things on their own pace...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111397