This is an archive of the discontinued LLVM Phabricator instance.

[hip] Add noalias on restrict qualified coerced hip pointers
AbandonedPublic

Authored by kerbowa on Apr 30 2020, 3:22 PM.

Details

Diff Detail

Event Timeline

kerbowa created this revision.Apr 30 2020, 3:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2020, 3:22 PM
kerbowa updated this revision to Diff 261388.Apr 30 2020, 4:30 PM

Fix test formatting.

yaxunl added inline comments.Apr 30 2020, 4:51 PM
clang/lib/CodeGen/CGCall.cpp
2267

let's also check LangOpts.CudaIsDevice

2268

let's use getContext().getTargetAddressSpace(LangAS::Default) for addrspace 0 and getContext().getTargetAddressSpace(LangAS::cuda_device) for addrspace 1

2270

For struct containing pointers, we do recursive coercing

https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/TargetInfo.cpp#L8224

So solely compare element type will fail. We need to add a test case for struct containing pointer, and we need to have a recursive comparison in a similar way as above code.

kerbowa added inline comments.Apr 30 2020, 5:29 PM
clang/lib/CodeGen/CGCall.cpp
2270

I can add it, but are we sure it's what we want? I think OpenCL/hcc wont have the same behavior because of https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CGCall.cpp#L2435.

yaxunl added inline comments.Apr 30 2020, 5:44 PM
clang/lib/CodeGen/CGCall.cpp
2270

Michael said without that there were extra flat load/stores. That's why he did that intentionally.

kerbowa added inline comments.Apr 30 2020, 6:15 PM
clang/lib/CodeGen/CGCall.cpp
2270

Okay. What I mean is that OpenCL/hcc don't add noalias recursively to struct members that are restrict qualified. Is there any reason not to do it?

hliao added a comment.EditedApr 30 2020, 9:24 PM

Basically, I think that should be a generic issue. As argument coercing doesn't handle pointer previously, we need to address this pointer-specific issue. That is, if the original argument has qualifiers being able to map onto LLVM attributes, the coerced argument should be those qualifiers as well if the new coerced one is still a pointer as the original one. We may lose more useful attributes not limited to noalias, such as alignment, nonnull, and etc.

clang/lib/CodeGen/CGCall.cpp
2556–2563

I don't think we need to check pointer address and/or HIP specific. As the generic argument processing, if the original type has any qualifiers, the coerced type (if it has a single value as the original parameter) should have those qualifiers as well. Here, we not only miss restrict but also alignment, nonnull, and etc. It should be fixed as a generic case instead of a target- or language-specific one.

yaxunl added inline comments.May 1 2020, 8:22 AM
clang/lib/CodeGen/CGCall.cpp
2556–2563

I agree we should migrate other argument properties for promoted pointer-type kernel arg for HIP, and that should be possible since other than the address space change, the type is the same.

However, I am not sure if we can do that for coerced function arguments in general. It may not even be pointer any more.

hliao added inline comments.May 1 2020, 10:38 AM
clang/lib/CodeGen/CGCall.cpp
2556–2563

If the coerced type is still a pointer but diffs on the element type, address space, or others. They should share the same qualifiers for pointers.

hliao added a comment.May 4 2020, 2:22 PM

Any more comments? As this should be a performance-critical issue, shall we get conclusion and make progress for the next step?

Any more comments? As this should be a performance-critical issue, shall we get conclusion and make progress for the next step?

We applied this current version of the patch internally for testing which is the reason for the lack of urgency, but hopefully we can get the final generic solution submitted upstream early this week. I believe I understand your comments and it makes sense to me that it should be safe for pointers whose coerced type is also a single pointer to retain the same attributes.

rjmccall added inline comments.May 4 2020, 7:28 PM
clang/lib/CodeGen/CGCall.cpp
2556–2563

If the parameter type is restrict-qualified and there's a single IR argument of pointer type, you should be able to apply NoAlias, regardless of coercion, in all language modes. You can just hoist that out of the "trivial case" block.

hliao added a comment.May 4 2020, 10:51 PM

I hoisted parameter attributes preparation in D79395, which depends on D79394, a cleanup to make that hoist more straight-forward.

kerbowa abandoned this revision.May 5 2020, 1:04 PM