This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9f6250f59105: [Clang][AArch64][SME] Add vector load/store
(ld1/st1) intrinsics (authored by bryanpkc).
Repository:
rG LLVM Github Monorepo
sdesmalen accepted this revision.
sdesmalen added a comment.
Thanks @bryanpkc this patch looks good to me now. I'll make some time to review
the other patches in the series as well after you update them.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
bryanpkc added a comment.
@sdesmalen How does the latest version look to you? If you approve, I will land
this and finish updating the rest of the patch series. One of the pre-merge
builds failed for an unrelated reason, but other jobs were successful.
Repository:
rG LLVM Github Monorepo
bryanpkc updated this revision to Diff 522064.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127910/new/
https://reviews.llvm.org/D127910
Files:
clang/include/clang/Basic/BuiltinsAArch64.def
clang/include/clang/Basic/BuiltinsARM.def
bryanpkc updated this revision to Diff 522039.
bryanpkc edited the summary of this revision.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127910/new/
https://reviews.llvm.org/D127910
Files:
clang/include/clang/Basic/BuiltinsNEON.def
bryanpkc added inline comments.
Comment at: clang/lib/Basic/Targets/AArch64.cpp:726
-if (Feature == "+sme") {
- HasSME = true;
sdesmalen wrote:
> Why did you remove this?
The `Feature == "+sme"` case is also handled below on line 782. Perhaps we
sdesmalen added inline comments.
Comment at: clang/lib/Basic/Targets/AArch64.cpp:726
-if (Feature == "+sme") {
- HasSME = true;
Why did you remove this?
Comment at: clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_ld1.c:16
+//
bryanpkc updated this revision to Diff 500638.
bryanpkc edited the summary of this revision.
bryanpkc added a comment.
Fix some bugs in the range checks of immediate operands.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127910/new/
bryanpkc updated this revision to Diff 500630.
bryanpkc added a comment.
Moved test case `acle_target_sme.c` into
`clang/test/Sema/aarch64-sme-intrinsics/`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127910/new/
https://reviews.llvm.org/D127910
Files:
bryanpkc updated this revision to Diff 500578.
bryanpkc added a comment.
Removed `EltTypeBool128` as suggested.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127910/new/
https://reviews.llvm.org/D127910
Files:
kmclaughlin accepted this revision.
kmclaughlin added a comment.
This revision is now accepted and ready to land.
Thank you for checking and removing EltTypeBool128. I think you have addressed
all of the other comments on this patch too, so it looks good to me!
Please can you update the commit
bryanpkc marked an inline comment as done.
bryanpkc added inline comments.
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:8874
case SVETypeFlags::EltTyBool64:
+ case SVETypeFlags::EltTyBool128:
return Builder.getInt1Ty();
kmclaughlin wrote:
> Is it
kmclaughlin added inline comments.
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:8874
case SVETypeFlags::EltTyBool64:
+ case SVETypeFlags::EltTyBool128:
return Builder.getInt1Ty();
Is it necessary to add an `EltTypeBool128`? I think the
bryanpkc marked an inline comment as done.
bryanpkc added inline comments.
Comment at: clang/lib/Headers/CMakeLists.txt:332
+ # Generate arm_sme.h
+ clang_generate_header(-gen-arm-sme-header arm_sme.td arm_sme.h)
# Generate arm_bf16.h
sdesmalen wrote:
>
bryanpkc updated this revision to Diff 498700.
bryanpkc marked 2 inline comments as done and an inline comment as not done.
bryanpkc added a comment.
Addressed review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127910/new/
sdesmalen added inline comments.
Comment at: clang/lib/Headers/CMakeLists.txt:332
+ # Generate arm_sme.h
+ clang_generate_header(-gen-arm-sme-header arm_sme.td arm_sme.h)
# Generate arm_bf16.h
bryanpkc wrote:
> bryanpkc wrote:
> > sdesmalen wrote:
> > > The
dmgreen added inline comments.
Comment at: clang/utils/TableGen/SveEmitter.cpp:1477
+
+ OS << "#if !defined(__ARM_FEATURE_SME)\n";
+ OS << "#error \"SME support not enabled\"\n";
bryanpkc wrote:
> dmgreen wrote:
> > We have been changing how the existing SVE
bryanpkc updated this revision to Diff 496976.
bryanpkc marked an inline comment as done.
bryanpkc added a comment.
Fixed minor bugs in the previous upload.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127910/new/
https://reviews.llvm.org/D127910
bryanpkc marked 5 inline comments as done.
bryanpkc added inline comments.
Comment at: clang/include/clang/Basic/arm_sme.td:22
+let TargetGuard = "sme" in {
+ def SVLD1_HOR_ZA8 : MInst<"svld1_hor_za8", "vimiPQ", "c", [IsLoad,
IsOverloadNone, IsStreaming, IsSharedZA],
bryanpkc updated this revision to Diff 496964.
bryanpkc edited the summary of this revision.
bryanpkc added a comment.
Addressed review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127910/new/
https://reviews.llvm.org/D127910
Files:
bryanpkc added inline comments.
Comment at: clang/lib/Headers/CMakeLists.txt:332
+ # Generate arm_sme.h
+ clang_generate_header(-gen-arm-sme-header arm_sme.td arm_sme.h)
# Generate arm_bf16.h
sdesmalen wrote:
> The ACLE specification is still in a draft
sdesmalen added inline comments.
Comment at: clang/lib/Headers/CMakeLists.txt:332
+ # Generate arm_sme.h
+ clang_generate_header(-gen-arm-sme-header arm_sme.td arm_sme.h)
# Generate arm_bf16.h
The ACLE specification is still in a draft (ALP) state, which
david-arm added a comment.
Thanks a lot for making all the changes @bryanpkc - it's looking really good
now! I just have a few minor comments/suggestions and then I think it looks
good to go.
Comment at: clang/include/clang/Basic/arm_sme.td:22
+let TargetGuard = "sme" in {
+
bryanpkc updated this revision to Diff 495592.
bryanpkc marked 4 inline comments as done.
bryanpkc added a comment.
Rebased on trunk and addressed review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127910/new/
bryanpkc marked 9 inline comments as done.
bryanpkc added inline comments.
Comment at: clang/include/clang/Basic/TargetBuiltins.h:312
+ /// Flags to identify the types for overloaded SME builtins.
+ class SMETypeFlags {
+uint64_t Flags;
david-arm wrote:
>
dmgreen added inline comments.
Comment at: clang/utils/TableGen/SveEmitter.cpp:1477
+
+ OS << "#if !defined(__ARM_FEATURE_SME)\n";
+ OS << "#error \"SME support not enabled\"\n";
We have been changing how the existing SVE and NEON instrinsics work a little.
david-arm added a comment.
Hi @bryanpkc, this is looking a lot better now and thanks for addressing the
comments! I've not reviewed all of the patch yet, but I do have a few more
comments. The most important ones are about performing immediate range checks
for the builtins and not declaring
bryanpkc updated this revision to Diff 492586.
bryanpkc added a comment.
Minor clean-up. Sorry for the noise.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127910/new/
https://reviews.llvm.org/D127910
Files:
bryanpkc updated this revision to Diff 492585.
bryanpkc retitled this revision from "[Clang][AArch64] Add SME C intrinsics for
load and store" to "[Clang][AArch64][SME] Add vector load/store (ld1/st1)
intrinsics".
bryanpkc edited the summary of this revision.
bryanpkc added a comment.
Updated
29 matches
Mail list logo
