[PATCH] D111397: RFC: A new way of skipping machine verification after problematic passes

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

As discussed in https://reviews.llvm.org/D111386 I have a feeling that some targets can likely be improved by moving code from `addPreEmitPass` to `addPreEmitPass2`. But we should probably discuss this independently of this patch, since I have a feeling that one way or another we won't end up with 100% of things fixed.

I am in favor of this change and just have some nitpicks.



================
Comment at: llvm/include/llvm/CodeGen/MachineFunction.h:152-166
+  // Verifies: Means that the function is expected to pass machine verification.
+  //  This can be cleared by passes that introduce known problems that have not
+  //  been fixed yet.
   enum class Property : unsigned {
     IsSSA,
     NoPHIs,
     TracksLiveness,
----------------
Can we use a flag where the default/desired state is not set?


================
Comment at: llvm/lib/CodeGen/MachineFunction.cpp:160
   Properties.set(MachineFunctionProperties::Property::TracksLiveness);
+  Properties.set(MachineFunctionProperties::Property::Verifies);
   if (STI->getRegisterInfo())
----------------
See above (when switching to `DisableVerification`).


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:296-297
+      // Skip functions that have known verification problems.
+      // FIXME: Remove this mechanism when all problematic passes have been
+      // fixed.
+      if (!MF.getProperties().hasProperty(
----------------
FWIW: I don't have high hopes for this flag ever disappearing once we introduce it and would tend to drop the comment (but no strong opinion you can keep it if you want).


================
Comment at: llvm/lib/Target/AMDGPU/AMDILCFGStructurizer.cpp:130
   bool runOnMachineFunction(MachineFunction &MF) override {
+    // FIXME: This pass causes verification failures.
+    MF.getProperties().reset(MachineFunctionProperties::Property::Verifies);
----------------
So, there's this thing I've seen other projects do, where they file tickets in the bugtracker and reference those in code. I know we don't usually do this in LLVM yet, but maybe we can set a new precedent here, what do you think?


================
Comment at: llvm/lib/Target/AMDGPU/SILowerControlFlow.cpp:790
 bool SILowerControlFlow::runOnMachineFunction(MachineFunction &MF) {
+  // FIXME: This pass causes verification failures.
+  MF.getProperties().reset(MachineFunctionProperties::Property::Verifies);
----------------
As above.


================
Comment at: llvm/lib/Target/Hexagon/HexagonVLIWPacketizer.cpp:206
 bool HexagonPacketizer::runOnMachineFunction(MachineFunction &MF) {
+  // FIXME: This pass causes verification failures.
+  MF.getProperties().reset(MachineFunctionProperties::Property::Verifies);
----------------
as above


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111397