Make conflicting properties a hard error instead of preferring the less strict one

Author: gargaroff

llvm/include/llvm/CodeGen/MIRYamlMapping.h

@@ -731,9 +731,9 @@ struct MachineFunction {
   bool HasWinCFI = false;
 
   // Computed properties that should be overridable
-  bool NoPHIs = false;
-  bool IsSSA = false;
-  bool NoVRegs = false;
+  std::optional<bool> NoPHIs;
+  std::optional<bool> IsSSA;
+  std::optional<bool> NoVRegs;
 
   bool CallsEHReturn = false;
   bool CallsUnwindInit = false;
@@ -777,9 +777,9 @@ template <> struct MappingTraits<MachineFunction> {
 
     // PHIs must be not be capitalized, since it will clash with the MIR opcode
     // leading to false-positive FileCheck hits with CHECK-NOT
-    YamlIO.mapOptional("noPhis", MF.NoPHIs, true);
-    YamlIO.mapOptional("isSSA", MF.IsSSA, true);
-    YamlIO.mapOptional("noVRegs", MF.NoVRegs, true);
+    YamlIO.mapOptional("noPhis", MF.NoPHIs, std::optional<bool>());
+    YamlIO.mapOptional("isSSA", MF.IsSSA, std::optional<bool>());
+    YamlIO.mapOptional("noVRegs", MF.NoVRegs, std::optional<bool>());
 
     YamlIO.mapOptional("callsEHReturn", MF.CallsEHReturn, false);
     YamlIO.mapOptional("callsUnwindInit", MF.CallsUnwindInit, false);

llvm/lib/CodeGen/MIRParser/MIRParser.cpp

@@ -178,7 +178,7 @@ class MIRParserImpl {
   SMDiagnostic diagFromBlockStringDiag(const SMDiagnostic &Error,
                                        SMRange SourceRange);
 
-  void computeFunctionProperties(MachineFunction &MF,
+  bool computeFunctionProperties(MachineFunction &MF,
                                  const yaml::MachineFunction &YamlMF);
 
   void setupDebugValueTracking(MachineFunction &MF,
@@ -374,7 +374,7 @@ static bool isSSA(const MachineFunction &MF) {
   return true;
 }
 
-void MIRParserImpl::computeFunctionProperties(
+bool MIRParserImpl::computeFunctionProperties(
     MachineFunction &MF, const yaml::MachineFunction &YamlMF) {
   MachineFunctionProperties &Properties = MF.getProperties();
 
@@ -401,25 +401,60 @@ void MIRParserImpl::computeFunctionProperties(
     }
   }
 
-  // Don't overwrite NoPHIs if the input MIR explicitly set it to false
-  if (YamlMF.NoPHIs && !HasPHI)
-    Properties.set(MachineFunctionProperties::Property::NoPHIs);
+  // Helper function to sanity-check and set properties that are computed, but
+  // may be explicitly set from the input MIR
+  auto ComputedPropertyHelper =
+      [&Properties](std::optional<bool> ExplicitProp, bool ComputedProp,
+                    MachineFunctionProperties::Property P) -> bool {
+    // Prefer whatever is set explicitly by the input MIR
+    if (ExplicitProp.has_value()) {
+      if (*ExplicitProp) {
+        // Check for conflicts with the computed value
+        if (!ComputedProp)
+          return true;
+
+        Properties.set(P);
+      } else
+        Properties.reset(P);
+
+      return false;
+    }
+
+    // No explicit value given, so use computed value
+    if (ComputedProp)
+      Properties.set(P);
+    else
+      Properties.reset(P);
+
+    return false;
+  };
+
+  if (ComputedPropertyHelper(YamlMF.NoPHIs, !HasPHI,
+                             MachineFunctionProperties::Property::NoPHIs)) {
+    return error(MF.getName() +
+                 " has explicit property NoPhi, but contains at least one PHI");
+  }
 
   MF.setHasInlineAsm(HasInlineAsm);
 
   if (HasTiedOps && AllTiedOpsRewritten)
     Properties.set(MachineFunctionProperties::Property::TiedOpsRewritten);
 
-  // Don't overwrite IsSSA if the input MIR explicitly set it to false
-  if (YamlMF.IsSSA && isSSA(MF))
-    Properties.set(MachineFunctionProperties::Property::IsSSA);
-  else
-    Properties.reset(MachineFunctionProperties::Property::IsSSA);
+  if (ComputedPropertyHelper(YamlMF.IsSSA, isSSA(MF),
+                             MachineFunctionProperties::Property::IsSSA)) {
+    return error(MF.getName() +
+                 " has explicit property IsSSA, but is not valid SSA");
+  }
 
-  // Don't overwrite NoVRegs if the input MIR explicitly set it to false
   const MachineRegisterInfo &MRI = MF.getRegInfo();
-  if (YamlMF.NoVRegs && MRI.getNumVirtRegs() == 0)
-    Properties.set(MachineFunctionProperties::Property::NoVRegs);
+  if (ComputedPropertyHelper(YamlMF.NoVRegs, MRI.getNumVirtRegs() == 0,
+                             MachineFunctionProperties::Property::NoVRegs)) {
+    return error(
+        MF.getName() +
+        " has explicit property NoVRegs, but contains virtual registers");
+  }
+
+  return false;
 }
 
 bool MIRParserImpl::initializeCallSiteInfo(
@@ -602,7 +637,8 @@ MIRParserImpl::initializeMachineFunction(const yaml::MachineFunction &YamlMF,
   MachineRegisterInfo &MRI = MF.getRegInfo();
   MRI.freezeReservedRegs();
 
-  computeFunctionProperties(MF, YamlMF);
+  if (computeFunctionProperties(MF, YamlMF))
+    return false;
 
   if (initializeCallSiteInfo(PFS, YamlMF))
     return false;

llvm/test/CodeGen/MIR/Generic/machine-function-optionally-computed-properties-conflict.mir

@@ -0,0 +1,38 @@
+# RUN: not llc -run-pass none -o /dev/null %s 2>&1 | FileCheck %s
+
+# Test that computed properties are not conflicting with explicitly set
+# properties
+
+---
+# CHECK-LABEL: TestNoPhisOverrideConflict
+# CHECK-SAME: has explicit property NoPhi, but contains at least one PHI
+name:            TestNoPhisOverrideConflict
+noPhis: true
+tracksRegLiveness: true
+body: |
+  bb.0:
+    %0:_(s32) = IMPLICIT_DEF
+
+  bb.1:
+    %1:_(s32) = PHI %0, %bb.0, %1, %bb.1
+    G_BR %bb.1
+...
+---
+# CHECK-LABEL: TestIsSSAOverrideConflict
+# CHECK-SAME: has explicit property IsSSA, but is not valid SSA
+name:            TestIsSSAOverrideConflict
+isSSA: true
+body: |
+  bb.0:
+    %0:_(s32) = IMPLICIT_DEF
+    %0:_(s32) = IMPLICIT_DEF
+...
+---
+# CHECK-LABEL: TestNoVRegsOverrideConflict
+# CHECK-SAME: has explicit property NoVRegs, but contains virtual registers
+name:            TestNoVRegsOverrideConflict
+noVRegs: true
+body: |
+  bb.0:
+    %0:_(s32) = IMPLICIT_DEF
+...

llvm/test/CodeGen/MIR/Generic/machine-function-optionally-computed-properties.mir

@@ -1,9 +1,6 @@
 # RUN: llc -run-pass none -o - %s | FileCheck %s
-# Test that we can disable certain properties that are normally computed. This
-# override is a soft-override (only allows overriding when it relaxes the
-# property). Therefore, a conflicting override (e.g. setting noPhis to true in
-# the presence of PHI nodes) should not result in an error, but instead should
-# use the computed property instead (noPhis = false in the mentioned example).
+
+# Test that we can disable certain properties that are normally computed
 
 ---
 # CHECK-LABEL: name: TestNoPhis
@@ -26,21 +23,6 @@ name:            TestNoPhisOverrideTrue
 noPhis: true
 ...
 ---
-# CHECK-LABEL: name: TestNoPhisOverrideConflict
-# CHECK: noPhis: false
-# CHECK: ...
-name:            TestNoPhisOverrideConflict
-noPhis: true
-tracksRegLiveness: true
-body: |
-  bb.0:
-    %0:_(s32) = IMPLICIT_DEF
-
-  bb.1:
-    %1:_(s32) = PHI %0, %bb.0, %0, %bb.1
-    G_BR %bb.1
-...
----
 # CHECK-LABEL: name: TestIsSSA
 # CHECK: isSSA: true
 # CHECK: ...
@@ -61,17 +43,6 @@ name:            TestIsSSAOverrideTrue
 isSSA: true
 ...
 ---
-# CHECK-LABEL: name: TestIsSSAOverrideConflict
-# CHECK: isSSA: false
-# CHECK: ...
-name:            TestIsSSAOverrideConflict
-isSSA: true
-body: |
-  bb.0:
-    %0:_(s32) = IMPLICIT_DEF
-    %0:_(s32) = IMPLICIT_DEF
-...
----
 # CHECK-LABEL: name: TestNoVRegs
 # CHECK: noVRegs: true
 # CHECK: ...
@@ -91,13 +62,3 @@ noVRegs: false
 name:            TestNoVRegsOverrideTrue
 noVRegs: true
 ...
----
-# CHECK-LABEL: name: TestNoVRegsOverrideConflict
-# CHECK: noVRegs: false
-# CHECK: ...
-name:            TestNoVRegsOverrideConflict
-noVRegs: true
-body: |
-  bb.0:
-    %0:_(s32) = IMPLICIT_DEF
-...