This is an archive of the discontinued LLVM Phabricator instance.

[Verifier] Fail on invalid indices for {insert,extract} vector intrinsics
AbandonedPublic

Authored by joechrisellis on May 20 2021, 5:21 AM.

Details

Summary

The langref (llvm/docs/LangRef.rst) specifies the following for the
llvm.experimental.vector.insert intrinsic:

``idx`` represents the starting element number at which ``subvec``
will be inserted. ``idx`` must be a constant multiple of
``subvec``'s known minimum vector length.

and the following for the llvm.experimental.vector.extract intrinsic:

The ``idx`` specifies the starting element number within ``vec``
from which a subvector is extracted. ``idx`` must be a constant
multiple of the known-minimum vector length of the result type.

These conditions were not previously enforced in the verifier, meaning
that the implementation was not entirely consistent with the langref. In
some circumstances, invalid indices were permitted silently, and in
other circumstances, an undef was spawned where a verifier error would
have been preferred.

This patch adds more checks to the verifier to ensure that these
constraints are not violated. It also updates existing tests to make
sure that they abide by the use of the intrinsic documented in the
langref.

Diff Detail

Event Timeline

joechrisellis created this revision.May 20 2021, 5:21 AM
joechrisellis requested review of this revision.May 20 2021, 5:22 AM
Herald added a project: Restricted Project. ยท View Herald TranscriptMay 20 2021, 5:22 AM
Herald added a subscriber: llvm-commits. ยท View Herald Transcript
joechrisellis retitled this revision from [Verifier] Fail on invalid indices in {insert,extract} vector intrinsics to [Verifier] Fail on invalid indices for {insert,extract} vector intrinsics.May 20 2021, 5:26 AM

Reviewers, especially @david-arm: there are still additional verifier checks to be implemented. In particular, we can add a verifier check for 'overrunning' the vectors with an insert/extract. This is currently handled by spawning an undef at the moment, but where we can determine this statically it makes sense to have the verifier spit an error. ๐Ÿ™‚

david-arm added inline comments.May 21 2021, 4:48 AM
llvm/test/CodeGen/AArch64/sve-extract-vector.ll
15

I think the name needs updating to idx2 now?

48

Same here

llvm/test/CodeGen/AArch64/sve-insert-vector.ll
112

This is just FYI and not caused by your patch, but suppose vscale = 1 and we clamp the index of 8 to 7. The str q1, [x9, x8] instruction actually ends up storing out 128 bits starting from index 7, which is beyond the end of temporary stack space. It looks like we could be corrupting the stack!

joechrisellis marked 2 inline comments as done.

Address review comments.

joechrisellis added inline comments.
llvm/test/CodeGen/AArch64/sve-insert-vector.ll
112

Hi Dave, thanks for pointing this out -- this is a great spot. Had a chat with @paulwalker-arm about this and will be looking into what the possibilities are here. A key takeaway from our conversation was that if vscale = 1 for this particular insertion then we're not strictly performing an 'insert subvector', so in some sense we could consider this abuse of the intrinsic. Most of our use-cases for the intrinsic are with an idx of 0 as well, so I don't think this will cause problems if the compilation pipeline is run end-to-end on a C program or something.

Of course, we can't really defend against something like this statically, so the possibilities are limited. We discussed:

  • more stringent checks at runtime -- but this could easily result in poor code quality. Perhaps there is a different/better way of checking the bounds/clamping.
  • allocating more stack space than strictly necessary to give us some breathing room (? -- might have misunderstood this).

(@paulwalker-arm, if I have misunderstood anything let me know ๐Ÿ˜„)

paulwalker-arm added inline comments.May 21 2021, 9:06 AM
llvm/test/CodeGen/AArch64/sve-insert-vector.ll
112

I'll say it's not about "breathing room" but more guaranteeing there's enough stack space, so something like max(sizeof(ResultVT), sizeof(SubVT) + clamped(idx)*sizeof(Elt)) noting that this is only a problem when insert a fixed-length vector into a scalable vector.

The patch looks good thanks @joechrisellis! Just one comment left ...

llvm/lib/IR/Verifier.cpp
5314

Hi @joechrisellis could you add a verifier test for this new Assert too?

Matt added a subscriber: Matt.May 31 2021, 9:43 AM

Fixup RISCV tests.

The removed functions were not valid uses of the intrinsic -- this was
previously never flagged by the verifier.

joechrisellis marked an inline comment as done.Jun 1 2021, 3:11 AM
joechrisellis added inline comments.
llvm/lib/IR/Verifier.cpp
5314

Thanks for pointing this out -- turns out that this assertion is unnecessary because the intrinsic is defined like so:

def int_experimental_vector_insert : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
                                                           [LLVMMatchType<0>, llvm_anyvector_ty, llvm_i64_ty],
                                                           [IntrNoMem, ImmArg<ArgIndex<2>>]>;

The LLVMMatchType here means that the constraint described by this assertion is already enforced -- there must be an existing mechanism that does this for the intrinsics.

I'll remove this assertion. I think it's safe to assume that the generic checking mechanism for LLVMMatchType is already tested, so no need to add additional tests. ๐Ÿ˜„

joechrisellis marked an inline comment as done.

Address review comments.

Gentle ping. ๐Ÿ™‚

david-arm added inline comments.Jun 17 2021, 1:28 AM
llvm/test/CodeGen/RISCV/rvv/fixed-vectors-insert-subvector.ll
92 โ†—(On Diff #348920)

Is it worth keeping this test, but changing the index of 4 to 8?

frasercrmck added inline comments.Jun 17 2021, 1:39 AM
llvm/test/CodeGen/RISCV/rvv/fixed-vectors-insert-subvector.ll
92 โ†—(On Diff #348920)

Don't think so because the test below uses an index of 8. Axe it, I say. And thanks for catching this!

david-arm accepted this revision.Jun 17 2021, 1:41 AM

LGTM! Thanks for addressing the review comments!

This revision is now accepted and ready to land.Jun 17 2021, 1:41 AM
paulwalker-arm added inline comments.Jun 17 2021, 3:44 AM
llvm/lib/IR/Verifier.cpp
5320

I don't understand this assert and the matching one for experimental_vector_extract. When inserting a fixed length vector into a scalable vector we allow any index, assuming we are matching the logic of ISD::INSERT_SUBVECTOR. If you look at SelectionDAG::getNode you see the set of asserts we use there.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
1866โ€“1868

I appreciate I'm late to the party here but why do we bother transforming illegal usage? I would have thought it better to keep the intrinsic and let the verifier report an error.

By transforming the illegal usage to an undef we're just hiding a bug, which might cause people to waste time investigating a non-related area.

joechrisellis added inline comments.Jun 17 2021, 4:01 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
1866โ€“1868

There is a second patch here which I think addresses this concern: https://reviews.llvm.org/D102907

paulwalker-arm added inline comments.Jun 17 2021, 4:08 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
1866โ€“1868

Any objections to me recommending you merge this patch with D102907 and just have a single patch to fix the verification of these intrinsics.

joechrisellis marked an inline comment as done.Jun 17 2021, 5:37 AM
joechrisellis added inline comments.
llvm/lib/IR/Verifier.cpp
5320

Discussed offline -- repeating the conclusion here for clarity.

LangRef and the ISD node donโ€™t make a distinction between the โ€˜inserting fixed into scalableโ€™ and โ€˜inserting fixed into fixedโ€™ case as far as the index constraints are concerned. As a result, we thought it might be better to avoid implementing something that was not documented as a thing that is supported.

So will leave this as-is for now. It's easy enough to relax in the future if need be.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
1866โ€“1868

Nope, I can do that. ๐Ÿ‘

joechrisellis marked an inline comment as done.Jun 17 2021, 8:58 AM

Thanks for the reviews guys.

Abandoning in favour of D104468.

Cheers!

joechrisellis abandoned this revision.Jun 18 2021, 1:31 AM