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

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

Hi Graham,

I think this patch is difficult to review. It covers many different source files with only a small unit test to check the correctness. This isn't very robust against future changes and it makes it hard to know exactly what is and isn't supported.

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.

We would eventually end up with something like this patch, but I think that an incremental approach would have the advantage of quicker reviews with more focused discussions, more regression tests, and we would know how much was supported at any point. Does anyone else have a similar opinion?

F9618170: simplest.ll <https://reviews.llvm.org/F9618170>


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53137