Page MenuHomePhabricator

Implementation of intrinsic and SDNode definitions for VP load, store, gather, scatter.
Needs ReviewPublic

Authored by hussainjk on Thu, Mar 25, 10:56 AM.

Details

Reviewers
simoll
vkmr
Summary

This patch adds intrinsic definitions and SDNodes for predicated load/store/gather/scatter, based on the work done in D57504.

Diff Detail

Event Timeline

hussainjk created this revision.Thu, Mar 25, 10:56 AM
hussainjk requested review of this revision.Thu, Mar 25, 10:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Mar 25, 10:56 AM
vkmr added a comment.Fri, Mar 26, 10:22 AM

Thanks Hussain for this patch!
For now I have a couple of minor comments, I will take a deeper look later.
Also, I don't quite understand yet how VP handles (or plans to handle) specifying alignment parameter. Does it need VPBuilder (not to be confused with VPlan VPBuilder!) for that? Would it make sense to add the Alignment parameter to VP load/store/scatter/gather intrinsics (much like their masked counterparts) for now, unless there is already a way to specify that?

llvm/include/llvm/IR/Intrinsics.td
1341

Better to use DefaultAttrsIntrinsic? (Ditto for other load, gather and scatter)

1366

minor style nit: A blank line above TODO. Perhaps reposition it to above the scope where it applies.

Thanks Hussain for this patch!
For now I have a couple of minor comments, I will take a deeper look later.
Also, I don't quite understand yet how VP handles (or plans to handle) specifying alignment parameter. Does it need VPBuilder (not to be confused with VPlan VPBuilder!) for that? Would it make sense to add the Alignment parameter to VP load/store/scatter/gather intrinsics (much like their masked counterparts) for now, unless there is already a way to specify that?

We should use the align(N) attribute on pointers to convey the alignment. That's what we do in the LLVM for SX-Aurora stack (https://github.com/sx-aurora-dev/llvm-project/blob/683f41073689262a8a493cce7caba13bbccea56c/llvm/test/CodeGen/VE/Packed/vp_load_packed.ll#L80) . I'd say that the alignment parameter in the pre-existing masked.load/store simply predates that attribute.
And yes, setting alignment is something that the VPBuilder or what's-it-called-after-bikeshedding would take care of for you.

simoll added a project: Restricted Project.Mon, Mar 29, 6:05 AM
simoll added inline comments.Mon, Mar 29, 7:53 AM
llvm/unittests/IR/VPIntrinsicTest.cpp
47–52

There are no mask or vlen attributes in upstream.

Can we split this up into a patch with only the intrinsics and a second patch with SelectionDAG?

hussainjk updated this revision to Diff 334705.Thu, Apr 1, 8:09 AM

Reverted some SDNode bits.

hussainjk updated this revision to Diff 334719.Thu, Apr 1, 8:55 AM

Various code review fixes.

craig.topper added inline comments.
llvm/include/llvm/CodeGen/SelectionDAGNodes.h
1367

Can we override clang-format here? The previous code had a more consistent structure.

llvm/include/llvm/IR/IntrinsicInst.h
402

It kind of feels like maybe there should be a VPMemIntrinsic and a VPStoreIntrinsic subclasses that contain these methods? Then you can use the more common isa, cast, dyn_cast and not have Optional return values.

llvm/lib/IR/IntrinsicInst.cpp
299

Doesn't the getValue() below already assert if there is no value?

Just some minor comments. I think Craig's suggestion of mem subclasses sounds like it's worth investigating.

llvm/include/llvm/CodeGen/SelectionDAGNodes.h
1367

Alternatively, I suppose, it could be a switch statement (aside from the last two)? Presumably that'd be optimized well enough by compilers?

llvm/include/llvm/IR/IntrinsicInst.h
409

Inconsistent documentation style, e.g. return the and return The, and use of commas vs forward slashes.

Also, is clang-tidy correct here? No uppercase letter on static methods?

llvm/include/llvm/IR/Intrinsics.td
1346

How some the loads are IntrReadMem but the stores aren't IntrWriteMem?

llvm/include/llvm/IR/VPIntrinsics.def
91

typo: cannonical -> canonical

llvm/lib/IR/IntrinsicInst.cpp
305

Purely a matter of style but this is slightly shorter and (I think) a little clearer:

if (auto PtrParamOpt = GetMemoryPointerParamPos(getIntrinsicID()))
  return getArgOperand(PtrParamOpt.getValue());
return nullptr;