[PATCH] D109584: [VP] Implementing expansion pass for VP load and store.

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

In D109584#3352203 <https://reviews.llvm.org/D109584#3352203>, @loralb wrote:

> Changelog:
>
> - Address comments
>
> N.B.: the `IsUnmasked == true` path in `expandPredicationInMemoryIntrinsic()` is not reachable, as also shown by the tests. How should this be handled in the code? Do we still handle this case or we add something like `assert(!isAllTrueMask(MaskParam))` instead of defining the `IsUnmasked` boloean?

Is that because you're using scalable vectors and the `isAllTrueMask` is expecting a `ConstantVector`? To my mind we should either:

- use a better true-mask check (surely there's one we can reuse instead of implementing our own)
- add tests for fixed vectors
- use constantexpr all-ones masks in the scalable-vector tests

The last is a bit of a hack, but the other two sound reasonable. Adding tests for fixed vectors sounds like a good idea for this patch anyway, and it'd give us coverage of this code path. The first one should be done, but done in a separate patch. Just add a FIXME in the scalable-vector tests for now?


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

https://reviews.llvm.org/D109584