[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added inline comments.



Comment at: clang/include/clang/Basic/arm_sve.td:186
+def SVLDFF1   : MInst<"svldff1[_{2}]", "dPc", "csilUcUsUiUlhfd", [IsLoad], 
  MemEltTyDefault, "aarch64_sve_ldff1">;
+def SVLDFF1SB : MInst<"svldff1sb_{d}", "dPS", "silUsUiUl",   [IsLoad], 
  MemEltTyInt8,"aarch64_sve_ldff1">;
+def SVLDFF1UB : MInst<"svldff1ub_{d}", "dPW", "silUsUiUl",   [IsLoad, 
IsZExtReturn], MemEltTyInt8,"aarch64_sve_ldff1">;

Andrzej wrote:
> Tests for `ldff1sb` seem to be missing.
Good spot, I've added this test before committing the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

[Andrzej Warzynski via Phabricator via cfe-commits]

Andrzej added inline comments.



Comment at: clang/include/clang/Basic/arm_sve.td:186
+def SVLDFF1   : MInst<"svldff1[_{2}]", "dPc", "csilUcUsUiUlhfd", [IsLoad], 
  MemEltTyDefault, "aarch64_sve_ldff1">;
+def SVLDFF1SB : MInst<"svldff1sb_{d}", "dPS", "silUsUiUl",   [IsLoad], 
  MemEltTyInt8,"aarch64_sve_ldff1">;
+def SVLDFF1UB : MInst<"svldff1ub_{d}", "dPW", "silUsUiUl",   [IsLoad, 
IsZExtReturn], MemEltTyInt8,"aarch64_sve_ldff1">;

Tests for `ldff1sb` seem to be missing.


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

https://reviews.llvm.org/D76238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added a comment.

In D76238#1954585 , @SjoerdMeijer 
wrote:

> Thanks for the ping, hadn't noticed the updates.
>
> I may have also missed why you need the SVE_ACLE_FUNC macro. Can you quickly 
> comment why you need that? Just asking because in my opinion it doesn't 
> really make it more pleasant to read (so it's there for another reason).


Thanks, these are all good questions!

In the previous revision of the patch there were separate test files for the 
short-forms (overloaded forms) of the tests. For example:

  svint8_t svadd[_s8]_z(svbool_t pg, svint8_t op1, svint8_t op2)

has the fully specific intrinsic `svadd_s8_z`, but also the overloaded form 
`svadd_z`. With the SVE_ACLE_FUNC(..) macro we can test both variants with a 
different RUN line, one where the [_s] suffix is ignored, the other where all 
suffixes are concatenated, depending on whether SVE_OVERLOADED_FORMS is defined.

> On one of the other tickets (may have missed a notification there too)  I 
> asked why you need option `-fallow-half-arguments-and-returns`. Do you really 
> need that, and why?

I answered that on the other patch now. Sorry about that, forgot to click 
'submit' earlier.

> And last question is if you really need to pass these defines 
> `-D__ARM_FEATURE_SVE`?

`__ARM_FEATURE_SVE` is automatically enabled by the compiler when the compiler 
implements the ACLE spec and +sve is specified. Given that the compiler doesn't 
yet implement the spec, these tests add the feature macro explicitly. Once all 
builtins are upstreamed, we'll update these tests by removing the explicit 
`-D__ARM_FEATURE_SVE` flag.


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

https://reviews.llvm.org/D76238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Thanks for the ping, hadn't noticed the updates.

I may have also missed why you need the SVE_ACLE_FUNC macro. Can you quickly 
comment why you need that? Just asking because in my opinion it doesn't really 
make it more pleasant to read (so it's there for another reason).

On one of the other tickets (may have missed a notification there too)  I asked 
why you need option `-fallow-half-arguments-and-returns`. Do you really need 
that, and why?

And last question is if you really need to pass these defines 
`-D__ARM_FEATURE_SVE`?


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

https://reviews.llvm.org/D76238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added a comment.

@SjoerdMeijer are you happy with the changes I've made to the tests?


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

https://reviews.llvm.org/D76238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

Looks good to me. Please wait a day in case Eli has more comments.




Comment at: clang/include/clang/Basic/arm_sve.td:128
 def SVLD1   : MInst<"svld1[_{2}]", "dPc", "csilUcUsUiUlhfd", [IsLoad]>;
+def SVLD1SB : MInst<"svld1sb_{d}", "dPS", "silUsUiUl",   [IsLoad], 
 MemEltTyInt8>;
+def SVLD1UB : MInst<"svld1ub_{d}", "dPW", "silUsUiUl",   [IsLoad, 
IsZxtReturn], MemEltTyInt8>;

sdesmalen wrote:
> SjoerdMeijer wrote:
> > nit: don't think we have a coding style for tablegen, but it is exceeding 
> > 80 characters, even making this on phabricator a bit difficult to read, 
> > perhaps you can reshuffle this a bit.
> I gave it a try to avoid these long lines, but found that this made the .td 
> file a lot less readable. If you don't have a strong opinion on this, I'd 
> prefer to keep it as-is.
Ok, thanks for trying.



Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_ld1sh.c:5
+//
+// ld1sh
+//

Nit: I really don't mind this, but this comment in its current form it doesn't 
add much (might as well remove it, or add a description, same for the other 
files)


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

https://reviews.llvm.org/D76238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen marked 4 inline comments as done.
sdesmalen added inline comments.



Comment at: clang/include/clang/Basic/arm_sve.td:128
 def SVLD1   : MInst<"svld1[_{2}]", "dPc", "csilUcUsUiUlhfd", [IsLoad]>;
+def SVLD1SB : MInst<"svld1sb_{d}", "dPS", "silUsUiUl",   [IsLoad], 
 MemEltTyInt8>;
+def SVLD1UB : MInst<"svld1ub_{d}", "dPW", "silUsUiUl",   [IsLoad, 
IsZxtReturn], MemEltTyInt8>;

SjoerdMeijer wrote:
> nit: don't think we have a coding style for tablegen, but it is exceeding 80 
> characters, even making this on phabricator a bit difficult to read, perhaps 
> you can reshuffle this a bit.
I gave it a try to avoid these long lines, but found that this made the .td 
file a lot less readable. If you don't have a strong opinion on this, I'd 
prefer to keep it as-is.


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

https://reviews.llvm.org/D76238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/include/clang/Basic/AArch64SVETypeFlags.h:72
+  bool isStructStore() const { return Flags & IsStructStore; }
+  bool isZxtReturn() const { return Flags & IsZxtReturn; }
+

SjoerdMeijer wrote:
> nit: this one is non obvious (at least to me), so perhaps worth a comment 
> what this is. I can guess that Zext means zero extending, but you know, the 
> context...
"Zxt" is not a standard abbreviation in LLVM: please use "Zext", or 
"ZeroExtend".



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7471
+
+Value *CodeGenFunction::EmitSVEMaskedStore(llvm::Type *ReturnTy,
+   SmallVectorImpl &Ops,

ReturnTy is unused?



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7475
+  llvm::PointerType *PTy = cast(Ops[1]->getType());
+  llvm::Type *MemEltTy = PTy->getPointerElementType();
+

Please avoid using getPointerElementType where possible... it will interfere 
with opaque pointer types work.  (You should be able to get the element type 
from the AST.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7467
+
+  return IsZxtReturn ? Builder.CreateZExt(Load, VectorTy)
+ : Builder.CreateSExt(Load, VectorTy);

sdesmalen wrote:
> SjoerdMeijer wrote:
> > nit: and now looking at this, this can be a zero or sign extend, so `Zxt` 
> > is slightly misleading?
> The default is sign-extend, so we added a flag for the case where a 
> zero-extend is needed/expected. Are you suggesting to rename the flag or to 
> add an extra flag for IsSxt?
I don't know to be honest, it was a nit anyway, but whatever you think is best. 
But just reading the name `IsZxtReturn`, I was only expecting a zero-extend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen marked 2 inline comments as done.
sdesmalen added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7467
+
+  return IsZxtReturn ? Builder.CreateZExt(Load, VectorTy)
+ : Builder.CreateSExt(Load, VectorTy);

SjoerdMeijer wrote:
> nit: and now looking at this, this can be a zero or sign extend, so `Zxt` is 
> slightly misleading?
The default is sign-extend, so we added a flag for the case where a zero-extend 
is needed/expected. Are you suggesting to rename the flag or to add an extra 
flag for IsSxt?



Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_st1.c:314
+  // CHECK: @llvm.masked.store.nxv8i16.p0nxv8i16( %data, 
* %[[GEP]], i32 1,  %{{.*}})
+  return svst1_vnum_u16(pg, base, 129, data);
+}

I can probably prune these tests a bit, when we generated these initially we 
also tested for code-gen, (hence the 129, which doesn't fit the immediate). 
This is properly tested in .ll tests in LLVM now, so I'll remove these.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76238: [SveEmitter] Implement builtins for contiguous loads/stores

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Some nits inlined




Comment at: clang/include/clang/Basic/AArch64SVETypeFlags.h:72
+  bool isStructStore() const { return Flags & IsStructStore; }
+  bool isZxtReturn() const { return Flags & IsZxtReturn; }
+

nit: this one is non obvious (at least to me), so perhaps worth a comment what 
this is. I can guess that Zext means zero extending, but you know, the 
context...



Comment at: clang/include/clang/Basic/arm_sve.td:128
 def SVLD1   : MInst<"svld1[_{2}]", "dPc", "csilUcUsUiUlhfd", [IsLoad]>;
+def SVLD1SB : MInst<"svld1sb_{d}", "dPS", "silUsUiUl",   [IsLoad], 
 MemEltTyInt8>;
+def SVLD1UB : MInst<"svld1ub_{d}", "dPW", "silUsUiUl",   [IsLoad, 
IsZxtReturn], MemEltTyInt8>;

nit: don't think we have a coding style for tablegen, but it is exceeding 80 
characters, even making this on phabricator a bit difficult to read, perhaps 
you can reshuffle this a bit.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7467
+
+  return IsZxtReturn ? Builder.CreateZExt(Load, VectorTy)
+ : Builder.CreateSExt(Load, VectorTy);

nit: and now looking at this, this can be a zero or sign extend, so `Zxt` is 
slightly misleading?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76238



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits