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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
