[PATCH] D32530: [SVE][IR] Scalable Vector IR Type

[Graham Hunter via Phabricator via llvm-commits]

huntergr added a comment.

In D32530#1486007 <https://reviews.llvm.org/D32530#1486007>, @hfinkel wrote:

> In D32530#1485862 <https://reviews.llvm.org/D32530#1485862>, @rkruppe wrote:
>
> > In D32530#1485818 <https://reviews.llvm.org/D32530#1485818>, @huntergr wrote:
> >
> > > In D32530#1484668 <https://reviews.llvm.org/D32530#1484668>, @hfinkel wrote:
> > >
> > > > Why implementation defined and not UB for the case where the index exceeds the runtime length? How do you intend to define this for SVE?
> > >
> > >
> > > SVE uses a predicate for indexed inserts and extracts. We generate that predicate by comparing a splat of the index against a stepvector (0,1,2,3...); if the index is out of range then the predicate will be all false.
> > >
> > > For a `mov` (insert), that results in an unmodified vector.
> > >
> > > For a `lastb` (extract), that extracts the last lane in the vector if no predicate bits are true.
> > >
> > > I don't know if RVV or SX-Aurora have similarly defined semantics. If the preference is to make it UB, that's fine.
> >
> >
> > I think this should be UB, or at least return poison. However, making it UB has the unfortunate side effect of making it illegal to hoist these operations out of conditionals (which currently isn't the case), so maybe poison is better.
> >
> > For me the main reason for making is UB (aside from generally being conservative) is that element insert/extract can be usefully legalized by putting the vector into a stack slot and accessing the affected element with scalar loads/stores -- in fact, that's the default legalization strategy in trunk. But in that setting an out-of-bounds lane index would become a wild read/write (= clear-cut UB), unless the legalization code jumps through extra hoops to add a bounds check or clamp the lane index into range. This wouldn't affect SVE or RVV since they would implement custom lowering anyway, but for other targets (especially if we eventually generalize insert/extract to accept variable lane positions and do so for all vector types for consistency) it could become an unnecessary headache. Though if we make insertelement with out-of-range lane index return poison instead of being UB, then at minimum clamping in insertelement is necessary to avoid the wild write.
> >
> > Aside: your proposed semantics for SVE would break the property `extractelt(insertelt(vec, i, elt), i) = elt` which instcombine (and others) most likely assumes. If we make the insertelt return poison (or even just undef, FWIW), that issue goes away.
>
>
> I think that making the return be a poison value is best.


Ok, will update again. Thanks.


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

https://reviews.llvm.org/D32530