This is an archive of the discontinued LLVM Phabricator instance.

[NVPTX] Expose LDU builtins
ClosedPublic

Authored by jchlanda on Mar 3 2023, 6:12 AM.

Details

Summary

Also check if native half types are supported to give more descriptive error message, without it clang only reports incorrect intrinsic return type.

Diff Detail

Event Timeline

jchlanda created this revision.Mar 3 2023, 6:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2023, 6:12 AM
jchlanda requested review of this revision.Mar 3 2023, 6:12 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 3 2023, 6:12 AM
tra added a comment.Mar 3 2023, 2:30 PM

Nice. Thank you!

clang/lib/CodeGen/CGBuiltin.cpp
18133–18138

I'd add a comment describing a meaning of the fields in the returned pair.

18145

Can we use the standard StringRef Name = getContext().BuiltinInfo.getName(BuiltinID); to figure out the builtin name?

18269–18271

Nit: this would be a bit more readable:

std::string BuiltinName{HalfSupport.second};
CGM.Error(E->getExprLoc(),  BuiltinName + " requires native half type support.")`

You may also consider changing returned BuiltinName to be std::string, so you would not need an explicit temp var.

clang/test/CodeGen/builtins-nvptx-native-half-type-err.c
4

I think we could've done it all in one run if we were to do both ldg and ldu in one function.

llvm/test/CodeGen/NVPTX/ldu-ldg.ll
33

Should alignment be 2 ?

60

Hmm. I wonder why we end up with u64 here and not b64. Not that it matters in this case, but it is a discrepancy vs. f16.

jchlanda updated this revision to Diff 502948.Mar 7 2023, 1:41 AM
jchlanda marked 3 inline comments as done.

Address PR comments

clang/lib/CodeGen/CGBuiltin.cpp
18145

Ha, had a feeling it would exist, couldn't find it. Thanks.

18269–18271

Done the std::string return, thanks.

llvm/test/CodeGen/NVPTX/ldu-ldg.ll
60

That is copy/paste sloppiness on my part, sorry. I've updated the test to check generated PTX, not just the labels, and fixed the values.

It generates correct kinds of loads, based on the type, the only discrepancy is that it doesn't distinguish between signed and unsigned loads, always choosing the unsigned variant. I think that's by design, at ISel there is a check if the load needs to be extended and a correct CVT instruction will be added.

tra added inline comments.Mar 7 2023, 10:41 AM
clang/lib/CodeGen/CGBuiltin.cpp
18267–18271

I think we can simplify it all further.

auto HasHalfSupport = [&](unsigned BuiltinID) {
    auto &Context = getContext();
    return Context.getLangOpts().NativeHalfType || !Context.getTargetInfo().useFP16ConversionIntrinsics();
}
...

if (!HasHalfSupport(BuiltinID)) {
      CGM.Error(E->getExprLoc(),   getContext().BuiltinInfo.getName(BuiltinID) + " requires native half type support.");
jchlanda updated this revision to Diff 503242.Mar 7 2023, 11:10 PM

Simplify the check for half tys support.

jchlanda added inline comments.Mar 7 2023, 11:11 PM
clang/lib/CodeGen/CGBuiltin.cpp
18267–18271

Done, although we need a string append there, StringRef and Twine would not work.

tra added inline comments.Mar 8 2023, 11:39 AM
clang/lib/CodeGen/CGBuiltin.cpp
18267–18271

We don't really need append().
CGM.Error(E->getExprLoc(), getContext().BuiltinInfo.getName(BuiltinID).str() + " requires native half type support."); works.

jchlanda updated this revision to Diff 503644.Mar 9 2023, 12:02 AM

append -> +

jchlanda marked 5 inline comments as done.Mar 9 2023, 12:03 AM
jchlanda added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
18267–18271

Done.

jchlanda marked an inline comment as done.Mar 14 2023, 4:04 AM

@tra is there anything else I should do for this patch? Thank you.

tra accepted this revision.Mar 14 2023, 9:57 AM

LGTM with a test nit.

llvm/test/CodeGen/NVPTX/ldu-ldg.ll
30

Nit. Function names usually checked with CHECK-LABEL.

This revision is now accepted and ready to land.Mar 14 2023, 9:57 AM
jchlanda updated this revision to Diff 505394.Mar 15 2023, 1:37 AM

Use CHECK-LABEL.

jchlanda marked an inline comment as done.Mar 15 2023, 1:37 AM
This revision was landed with ongoing or failed builds.Mar 15 2023, 1:44 AM
This revision was automatically updated to reflect the committed changes.