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

Tue Jul 23 04:21:24 PDT 2019

rovka added a comment.

In D53137#1597088 <https://reviews.llvm.org/D53137#1597088>, @huntergr wrote:

> Hi Diana,
>
> Thanks for the comments.
>
> In D53137#1597022 <https://reviews.llvm.org/D53137#1597022>, @rovka wrote:
>
> > 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.
>
>
> Yeah, I was worried about that -- this is basically the size queries alone without anything actually using scalable vectors. It demonstrates roughly where changes will be needed, but doesn't actually change the surrounding code to use e.g. getElementCount instead of getNumElements.
>
> > 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.
>
> An incremental approach sounds good; assuming nobody objects, I'll remove most of the code in this patch and just leave the core mechanism behind (in enforcing mode) and add in that test case. We can fill in the other cases as we enable codegen/acle/autovec in separate patches.

Actually my test case falls in the "replace getNumElements with getElementCount" category, rather than size queries per se. I don't think either of them is more important than the other, but you might run into the former while writing tests for the latter.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53137