This is an archive of the discontinued LLVM Phabricator instance.

[llvm][CodeGen][aarch64] Add contiguous prefetch intrinsics for SVE.
ClosedPublic

Authored by fpetrogalli on Feb 13 2020, 2:33 PM.

Details

Summary

The patch covers both register/register and register/immediate addressing modes.

Event Timeline

fpetrogalli created this revision.Feb 13 2020, 2:33 PM
Herald added a project: Restricted Project. · View Herald Transcript

This is not yet ready to review.

Replace <n x ... with <vscale x ... in the test.

The patch now is ready to review. It covers only the contiguous prefetches. I will create a separate revision for the gather prefetches.

fpetrogalli retitled this revision from [llvm][aarch64][sve] Prefetch intrinsics. [WIP] to [llvm][CodeGen][aarch64] Add continuous prefetch intrinsics for SVE..Feb 19 2020, 2:41 PM
fpetrogalli edited the summary of this revision. (Show Details)
fpetrogalli added reviewers: andwar, sdesmalen.
fpetrogalli marked an inline comment as done.Feb 19 2020, 2:48 PM
fpetrogalli added inline comments.
llvm/include/llvm/IR/IntrinsicsAArch64.td
1263–1265

Ideally, I would have liked to define this as something like:

def int_aarch64_sve_prf : Intrinsic<[], [PointerTo<llvm_anyvector_ty>,
                                         LLVMScalarOrSameVectorWidth<0, llvm_i1_ty>,
                                         llvm_i32_ty], [IntrArgMemOnly]>;

For some definition of PointerTo, but I am not aware of anything in the current intrinsic generator that can be used for that.

sdesmalen accepted this revision.Feb 21 2020, 10:51 AM

nit: in the title:
s/continuous/contiguous/

LGTM otherwise

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
4414

nit: Any reason you're using an anonymous namespace over static for these functions?

4424

nit: you can fold this into the condition above.

4432

instead of 128, can you use AArch64::SVEBitsPerBlock?

This revision is now accepted and ready to land.Feb 21 2020, 10:51 AM
fpetrogalli retitled this revision from [llvm][CodeGen][aarch64] Add continuous prefetch intrinsics for SVE. to [llvm][CodeGen][aarch64] Add contiguous prefetch intrinsics for SVE..Feb 21 2020, 11:20 AM
fpetrogalli marked 4 inline comments as done.Feb 21 2020, 12:30 PM
fpetrogalli added inline comments.
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
4414

I should have really used static to adhere to the style of the rest of the file... Done.

fpetrogalli marked an inline comment as done.

Address code review from @sdesmalen.

This revision was automatically updated to reflect the committed changes.
andwar added inline comments.Feb 24 2020, 6:03 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
4415

[nit] corresponding?

4468–4469

What's this check for? Wouldn't some hard-coded null-value be more idiomatic?

llvm/test/CodeGen/AArch64/sve-intrinsics-contiguous-prefetches.ll
101

Would it make sense to add checks for what's expected instead?

fpetrogalli marked 2 inline comments as done.Feb 24 2020, 1:58 PM
fpetrogalli added inline comments.
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
4468–4469

I think EVT() is there to simulate a nullptr, see http://llvm.org/doxygen/ValueTypes_8h_source.html:

struct EVT {
private:
  MVT V = MVT::INVALID_SIMPLE_VALUE_TYPE;
  Type *LLVMTy = nullptr;
 
public:
  constexpr EVT() = default;

Essentially, if statement checks that the MemVT is not { MVT::INVALID_SIMPLE_VALUE_TYPE, nullptr}.

llvm/test/CodeGen/AArch64/sve-intrinsics-contiguous-prefetches.ll
101

I guess you are right. We could miss a failure in the addressing mode generation if we have (at the same time) a failure in the rendering of the immediate constant... quite a remote possibility, which I am sure it will be caught in time in some other tests.

So I think we can leave it as it is.