[PATCH] D75297: [TypeSize] Allow returning scalable size in implicit conversion to uint64_t

Tue Mar 3 09:40:13 PST 2020

sdesmalen added a comment.

In D75297#1901560 <https://reviews.llvm.org/D75297#1901560>, @efriedma wrote:

> Even without any optimizations that are specifically SVE-aware, we're going to generate a lot of instructions with SVE types that aren't SVE intrinsics.  In particular, I assume plain C assignment operations are going to lower to "load" and "store" instructions.  So really, this is trading assertions for subtle miscompiles in a lot of cases.

That's right. Plain assignment operations will use the unpredicated load/store instructions. There are definitely optimiziations for those that we'd need to disable for scalable vectors.

> If you're trying to hit a certain deadline, maybe being unable to build any SVE code at all is worse than a bunch of subtle miscompiles?  That seems like a terrible tradeoff either way, though.

It's also about making progress on getting any support for SVE/SVE2 in Clang/LLVM whilst in the meantime addressing the fixed-width assumptions more structurally. The one doesn't necessarily have to block the other, we can do this in parallel. At the moment scalable vectors are not yet actually supported in LLVM, so it is not unexpected that things are broken while we work towards adding support for it. 
I agree though that the trade-off is not the most comfortable one, but by minimising the use of common parts of LLVM by implementing most functionality with custom intrinsics, a lot of that risk can be mitigated for the ACLE.

In D75297#1901615 <https://reviews.llvm.org/D75297#1901615>, @ctetreau wrote:

> I have concerns about introducing technical debt that we plan to pay off "soon". Life happens and things get deemed "good enough". If this patch is necessary for work to proceed on ACLE, perhaps those working on it should locally cherry pick this patch and do their work?

It depends on what you mean with proceeding. Keeping the assert means that LLVM can't be used for scalable vectors until we've fully upgraded the code-base to use the proper interfaces to TypeSize (and ElementCount, given D75478 <https://reviews.llvm.org/D75478>), which is a really significant migration effort. The upgrading also becomes harder than it needs to be because tests need to be written that don't trigger the assert. In the worst case, we may not be able to write any tests until significant upgrading has occurred (I think we've already seen this in some cases).

> Currently, this implicit conversion is an alias for getFixedSize(). Lots of code is written using this assumption, and changing the meaning of the conversion is only going to add bugs. In my opinion, the conversion should be removed; if this had been done previously then we wouldn't be in this position today. Failing that, we should fix the breakages, not hide them.

Yes, I agree we should remove the conversion, the question is more how we go about doing that. It is a trade-off between working until we've fixed up all uses of `getSize()` before we can build a simple example, vs gradually making an implementation we have more stable.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75297