[PATCH] D53137: Scalable type size queries (llvm)

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

In D53137#1597022 <https://reviews.llvm.org/D53137#1597022>, @rovka wrote:

> I would find it much easier to review with an incremental strategy based on regression tests. For instance, with ToT opt, the attached testcase fails (error: '%r' defined with type '<4 x i1>' but expected '<vscale x 4 x i1>'). I would add a patch to fix that, and maybe other similar, really simple cases. We could then proceed to more complex examples, run some of the passes that come after the vectorizer on them, and progressively fix the places required to make them pass, with focused tests for each hurdle that we run into. It shouldn't be too hard to reduce such snippets from the tests you've already been running.


+1 to incremental approach and more tests!

This change is mostly mechanical, but you're absolutely right we need to be aware of unwanted side effects. I wrongly assumed mechanical == NFC, but this clearly isn't. Thanks for the thorough review!

I think tests need to be strict on what it should support. Not necessarily test for *all* errors, but add test for the supported cases and some negative tests for the obvious unsupported stuff.

Each step of the way, revert negative tests when new features are added (and adding more tests, too!), we can make sure it's stable and robust.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53137