[PATCH] D46338: [GlobalISel][Legalizer] LegalizerInfo verifier: checking that legalization rules cover all type indices

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:99
+  }
+  const bool AllCovered = (FirstUncovered >= NumTypeIdxs);
+  DEBUG(dbgs() << ".. the first uncovered type index: " << FirstUncovered
----------------
@qcolombet
+ @dsanders 

I'll update the tests as requested in https://reviews.llvm.org/D46339 in a bit, in the meantime, I'm wondering, if it makes sense to check that we don't cover **more** type indices than there is as well, not just less.

It looks like in practice such mistakes are much less common, also, checking more type indices then needed fails much easier on an assertion while accessing an out of bounds item of the `LegalityQuery::Types` ArrayRef anyway. However, such a check is much more dynamic in a sense that it will only happen if there is a test that triggers a creation of such a query, while the verification proposed here despite not being strictly speaking static still catches issues as soon as a legalizer for a particular target is instantiated, so in practice, //any// test suffices.

What do you think?

Also, what do you think on the idea in general? I'm not particularly insisting that this patch is a good thing.


Repository:
  rL LLVM

https://reviews.llvm.org/D46338