This is an archive of the discontinued LLVM Phabricator instance.

[NVPTX] Expose float tys min, max, abs, neg as builtins
ClosedPublic

Authored by jchlanda on Jan 21 2022, 5:52 AM.

Details

Summary

Adds support for the following builtins:

  • abs, neg:
    • .bf16,
    • .bf16x2
  • min, max
    • {.ftz}{.NaN}{.xorsign.abs}.f16
    • {.ftz}{.NaN}{.xorsign.abs}.f16x2
    • {.NaN}{.xorsign.abs}.bf16
    • {.NaN}{.xorsign.abs}.bf16x2
    • {.ftz}{.NaN}{.xorsign.abs}.f32

Diff Detail

Event Timeline

jchlanda created this revision.Jan 21 2022, 5:52 AM
jchlanda requested review of this revision.Jan 21 2022, 5:52 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 21 2022, 5:52 AM
tra added a comment.Jan 21 2022, 11:13 AM

Looks good overall.
Please do check that the generated PTX does get assembled by ptxas.

There are few newer variants of these instructions that appear to be missing. E.g. {min/max}.xorsign.abs.
If you only intended to add instructions available in PTX-7.0, which, based on the constraints used in the patch, appears to be the case, I'd mention that in the commit log.

clang/test/CodeGen/builtins-nvptx.c
822 ↗(On Diff #401965)

I'd #define the magic values to give them sensible names.

llvm/include/llvm/IR/IntrinsicsNVVM.td
582–615

Nit: variant might work better here and below.

llvm/lib/Target/NVPTX/NVPTXIntrinsics.td
652

Ditto. Capacity->Variant.

If you only intended to add instructions available in PTX-7.0, which, based on the constraints used in the patch, appears to be the case, I'd mention that in the commit log.

Yeap, we were only going to bump up to 7.0, I don't mind adding the xorsign.abs, while I'm at it. Will update the diff shortly.

jchlanda updated this revision to Diff 405161.Feb 2 2022, 12:29 AM
jchlanda edited the summary of this revision. (Show Details)

Added xorsign.abs variant and test.

Please do check that the generated PTX does get assembled by ptxas.

ptxas is happy with asm generated from both math-intrins-sm86-ptx72.ll and math-intrins-sm80-ptx70.ll

jchlanda marked 2 inline comments as done.Feb 2 2022, 12:34 AM
jchlanda added inline comments.
clang/test/CodeGen/builtins-nvptx.c
822 ↗(On Diff #401965)

I've added #defs for those values, they are not strictly needed (as in the values don't really matter) as this is not being executed, but I agree, it makes for a better read of the test.

jchlanda marked an inline comment as done.Feb 2 2022, 1:57 AM
tra accepted this revision.Feb 2 2022, 10:18 AM

ptxas is happy with asm generated from both math-intrins-sm86-ptx72.ll and math-intrins-sm80-ptx70.ll

Thank you for checking that.

clang/test/CodeGen/builtins-nvptx.c
822 ↗(On Diff #401965)

I mostly had NaN/Inf in mind. Arbitrary numbers could remain literals. This is OK, too.

This revision is now accepted and ready to land.Feb 2 2022, 10:18 AM
jchlanda marked an inline comment as done.Feb 3 2022, 5:15 AM
tra added inline comments.Feb 9 2022, 11:08 AM
llvm/lib/Target/NVPTX/NVPTXTargetTransformInfo.cpp
162

The new 3-argument constructor above obviates the need for this one.

jchlanda added inline comments.Feb 9 2022, 11:20 AM
llvm/lib/Target/NVPTX/NVPTXTargetTransformInfo.cpp
162

I'm not sure if it does, the 3-way takes Intrinsic, while this one Instruction.

tra added inline comments.Feb 9 2022, 11:33 AM
llvm/lib/Target/NVPTX/NVPTXTargetTransformInfo.cpp
162

You're right. Sorry, my mistake.

jchlanda marked 2 inline comments as done.Feb 9 2022, 11:37 AM
jchlanda added inline comments.
llvm/lib/Target/NVPTX/NVPTXTargetTransformInfo.cpp
162

np

This revision was landed with ongoing or failed builds.Feb 23 2022, 1:57 PM
This revision was automatically updated to reflect the committed changes.
jchlanda marked an inline comment as done.Mar 1 2022, 6:39 AM

@tra thank you for landing the patches, it seems that the clang part (builtin declarations and tests) have been dropped, only llvm dir changes made it through. Is there any way I could fix it (same goes for the other two patches in this stack)?

tra added a comment.Mar 1 2022, 10:26 AM

@tra thank you for landing the patches, it seems that the clang part (builtin declarations and tests) have been dropped, only llvm dir changes made it through. Is there any way I could fix it (same goes for the other two patches in this stack)?

Somehow arc export | git apply didn't pick clang changes when I was transferring the patch from the phabricator. I'll re-fetch the patches and will land the missing pieces shortly.

I'm not sure how the you've submitted the patch to phabricator. In general, it works best when the patch is supplied as a gic commit diff, with the author metadata, etc.
Or via arc diff. See for the details. https://llvm.org/docs/Phabricator.html#phabricator-reviews

tra added a comment.Mar 1 2022, 11:20 AM

Missing clang-side changes have landed. Please check.

@tra thank you for landing the patches, it seems that the clang part (builtin declarations and tests) have been dropped, only llvm dir changes made it through. Is there any way I could fix it (same goes for the other two patches in this stack)?

Somehow arc export | git apply didn't pick clang changes when I was transferring the patch from the phabricator. I'll re-fetch the patches and will land the missing pieces shortly.

I'm not sure how the you've submitted the patch to phabricator. In general, it works best when the patch is supplied as a gic commit diff, with the author metadata, etc.
Or via arc diff. See for the details. https://llvm.org/docs/Phabricator.html#phabricator-reviews

I went with the web interface as described here: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
with git diff -U999999 ... didn't want to bite the bullet of arc, hoping that github PRs will soon be a thing.

All working now, thank you for resolving that so quickly.

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 12:20 AM
tra added a comment.Mar 2 2022, 11:42 AM

I went with the web interface as described here: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
with git diff -U999999 ... didn't want to bite the bullet of arc, hoping that github PRs will soon be a thing.

git show -U99999 should work even better as it would include author info and commit log message, so one would not need to re-enter it manually when importing the patch from phabricator.

In any case that's probably not the root cause of my error, just a minor inconvenience.
I still have no idea what went wrong during my initial commit, but I think I've learned the lesson and will start double checking the imported changes for completeness.