[PATCH] D91262: [AArch64][SVE] Allow C-style casts between fixed-size and scalable vectors

[Peter Waller via Phabricator via cfe-commits]

peterwaller-arm added inline comments.


================
Comment at: clang/lib/Sema/SemaCast.cpp:2222
   if (srcIsVector || destIsVector) {
+    // We can bitcast between SVE VLATs and VLSTs, and vice-versa.
+    if (Self.isValidSveBitcast(SrcType, DestType)) {
----------------
It's good to avoid use of pronouns such as "We" in comments like this, since a different reader might take a different interpretation of the word "We". Is "We" the software itself? Is it the company who wrote the condition? Better to rewrite it in a more direct way; in my suggestion it is written so that it's clearer the following statements are what allow it. Even better if you can include a reference to a spec indicating the background on why it is allowed.


================
Comment at: clang/lib/Sema/SemaCast.cpp:2762
+  // If either the src or dest are a vector, it's possible that we want to do an
+  // SVE bitcast. We can bitcast between SVE VLATs and VLSTs, and vice-versa.
+  if (SrcType->isVectorType() || DestType->isVectorType())
----------------
Another couple of uses of the word "we". Suggest taking Cullen's suggestion to combine the conditions and say:

"Allow bitcasting between compatible SVE vector types".


================
Comment at: clang/lib/Sema/SemaCast.cpp:2763
+  // SVE bitcast. We can bitcast between SVE VLATs and VLSTs, and vice-versa.
+  if (SrcType->isVectorType() || DestType->isVectorType())
+    if (Self.isValidSveBitcast(SrcType, DestType)) {
----------------
c-rhodes wrote:
> I think braces are recommended on the outer if as well, see: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
> 
> Although I suppose it could also be written as:
> 
> ```
> if ((SrcType->isVectorType() || DestType->isVectorType()) &&
>     Self.isValidSveBitcast(SrcType, DestType)) {
>   Kind = CK_BitCast;
>   return;
> }```
I don't understand why this is an || rather than an &&, please can you clarify? I would have expected they must both be vectors.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:7200
 
+/// Are the two types SVE-bitcast-compatible types? I.e. can we bitcast from the
+/// first SVE type (e.g. an SVE VLAT) to the second type (e.g. an SVE VLST)?
----------------
Use of "we".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91262