[PATCH] D57437: GlobalISel: Add assert that legalize mutation makes sense

Matt Arsenault via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 29 21:02:51 PST 2019


arsenm created this revision.
arsenm added reviewers: aemerson, paquette, aditya_nandakumar, volkan, dsanders.
Herald added subscribers: Petar.Avramovic, kristof.beyls, rovka, wdng.

I've repeatedly encountered bugs resulting from custom legalize
mutations returning nonsense legalize results, such as increasing the
number of elements for FewerElements. Add an assert function to make
sure the type to mutate to is consistent with the legalize action.


https://reviews.llvm.org/D57437

Files:
  lib/CodeGen/GlobalISel/LegalizerInfo.cpp


Index: lib/CodeGen/GlobalISel/LegalizerInfo.cpp
===================================================================
--- lib/CodeGen/GlobalISel/LegalizerInfo.cpp
+++ lib/CodeGen/GlobalISel/LegalizerInfo.cpp
@@ -58,6 +58,66 @@
   return OS;
 }
 
+#ifndef NDEBUG
+// Make sure the returned mutation makes sense for the match type.
+static bool mutationIsSane(const LegalizeRule &Rule,
+                           const LegalityQuery &Q,
+                           std::pair<unsigned, LLT> Mutation) {
+  const unsigned TypeIdx = Mutation.first;
+  const LLT OldTy = Q.Types[TypeIdx];
+  const LLT NewTy = Mutation.second;
+
+  switch (Rule.getAction()) {
+  case FewerElements:
+  case MoreElements: {
+    if (!OldTy.isVector())
+      return false;
+
+    if (NewTy.isVector()) {
+      if (Rule.getAction() == FewerElements) {
+        // Make sure the element count really decreased.
+        if (NewTy.getNumElements() >= OldTy.getNumElements())
+          return false;
+      } else {
+        // Make sure the element count really increased.
+        if (NewTy.getNumElements() <= OldTy.getNumElements())
+          return false;
+      }
+    }
+
+    // Make sure the element type didn't change.
+    return NewTy.getScalarType() == OldTy.getElementType();
+  }
+  case NarrowScalar:
+  case WidenScalar: {
+    if (OldTy.isVector()) {
+      // Number of elements should not change.
+      if (!NewTy.isVector() || OldTy.getNumElements() != NewTy.getNumElements())
+        return false;
+    } else {
+      // Both types must be vectors
+      if (NewTy.isVector())
+        return false;
+    }
+
+    if (Rule.getAction() == NarrowScalar)  {
+      // Make sure the size really decreased.
+      if (NewTy.getScalarSizeInBits() >= OldTy.getScalarSizeInBits())
+        return false;
+    } else {
+      // Make sure the size really increased.
+      if (NewTy.getScalarSizeInBits() <= OldTy.getScalarSizeInBits())
+        return false;
+    }
+
+    return true;
+  }
+  default:
+    return true;
+  }
+}
+#endif
+
 LegalizeActionStep LegalizeRuleSet::apply(const LegalityQuery &Query) const {
   LLVM_DEBUG(dbgs() << "Applying legalizer ruleset to: "; Query.print(dbgs());
              dbgs() << "\n");
@@ -65,12 +125,15 @@
     LLVM_DEBUG(dbgs() << ".. fallback to legacy rules (no rules defined)\n");
     return {LegalizeAction::UseLegacyRules, 0, LLT{}};
   }
-  for (const auto &Rule : Rules) {
+  for (const LegalizeRule &Rule : Rules) {
     if (Rule.match(Query)) {
       LLVM_DEBUG(dbgs() << ".. match\n");
       std::pair<unsigned, LLT> Mutation = Rule.determineMutation(Query);
       LLVM_DEBUG(dbgs() << ".. .. " << (unsigned)Rule.getAction() << ", "
                         << Mutation.first << ", " << Mutation.second << "\n");
+      assert(mutationIsSane(Rule, Query, Mutation) &&
+             "legality mutation invalid for match");
+
       assert((Query.Types[Mutation.first] != Mutation.second ||
               Rule.getAction() == Lower ||
               Rule.getAction() == MoreElements ||


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D57437.184247.patch
Type: text/x-patch
Size: 3035 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190130/ce10a8b8/attachment.bin>


More information about the llvm-commits mailing list