This is an archive of the discontinued LLVM Phabricator instance.

[LLVM][AArch64] Replace aarch64.sve.ldN by aarch64.sve.ldN.sret
ClosedPublic

Authored by CarolineConcatto on Aug 31 2022, 8:04 AM.

Details

Summary

This patch removes the intrinsic aarch64.sve.ldN from tablegen in favour of
using arch64.sve.ldN.sret.

Depends on: D133023

Diff Detail

Event Timeline

CarolineConcatto requested review of this revision.Aug 31 2022, 8:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2022, 8:04 AM
CarolineConcatto retitled this revision from [LLVM][AArch64] Replace aarch64.sve.ld by aarch64.sve.ldN.sret to [LLVM][AArch64] Replace aarch64.sve.ldN by aarch64.sve.ldN.sret.Aug 31 2022, 8:13 AM
CarolineConcatto added reviewers: sdesmalen, bsmith.
sdesmalen added inline comments.Aug 31 2022, 8:19 AM
llvm/test/CodeGen/AArch64/sve-intrinsics-ldN-reg+imm-addr-mode.ll
0

We don't want to keep these tests in their current form, instead they should be rewritten to use the sret forms explicitly. Because after this patch, we don't support code-gen for the old forms any more (when AutoUpgrade isn't run).

If you want to test the transformation from the old wide-vector -> sret forms, you can create some new LLVM IR -> LLVM IR tests by running only the auto-upgrade pass.

2–4

This doesn't look right, you seem to have heard-coded the index for the extractvalue.

CarolineConcatto edited the summary of this revision. (Show Details)Aug 31 2022, 8:35 AM
  • Fix the index in: Value *SRet = Builder.CreateExtractValue(NewLdCall, 0);

from 0 to the index variable I.

  • Add a upgrade test in llvm/test/Bitcode for aarch64.sve.ldN
  • Remove the code generated tests for aarch64.sve.ldN intrinsic

in llvm/test/CodeGen/AArch64

Replace

.Default(Intrinsic::not_intrinsic); by .Default(0);

in line 3886 in AutoUpgrade.cpp

Matt added a subscriber: Matt.Sep 1 2022, 11:13 AM
sdesmalen added inline comments.Sep 6 2022, 2:38 AM
llvm/lib/IR/AutoUpgrade.cpp
561

Are you sure that the type mangling has been guaranteed at this point? Because the following IR without the type suffix also compiles:

define <vscale x 32 x i8> @ld2.nxv32i8(<vscale x 16 x i1> %Pg, i8 *%base_ptr) {
%res = call <vscale x 32 x i8> @llvm.aarch64.sve.ld2(<vscale x 16 x i1> %Pg, i8 *%base_ptr)
ret <vscale x 32 x i8> %res
}

declare <vscale x 32 x i8> @llvm.aarch64.sve.ld2(<vscale x 16 x i1>, i8 *)

I assume you'er doing it to avoid it also matching the .sret case, so maybe you can test for either .nxv[0-9] or $.

  • Upgrade ldN intrinsics with short names
CarolineConcatto marked an inline comment as done.Sep 8 2022, 4:01 AM
  • s/ unsigned N = StringSwitch<Intrinsic::ID>/ unsigned N = StringSwitch<unsigned>/
sdesmalen accepted this revision.Sep 14 2022, 1:55 PM
sdesmalen added inline comments.
llvm/lib/IR/AutoUpgrade.cpp
561

nit: do you need to escape the dot (\\.) here too?

This revision is now accepted and ready to land.Sep 14 2022, 1:55 PM
llvm/test/CodeGen/AArch64/sve-intrinsics-ldN-reg+imm-addr-mode.ll