This is an archive of the discontinued LLVM Phabricator instance.

[NVVM] Update intrinsic definitions to include more attributes
ClosedPublic

Authored by jdoerfert on Sep 17 2021, 11:27 AM.

Details

Summary

A lot of NVVM intrinsics can use the default intrinsic attributes (e.g.,
nosync, nofree, ...) as well as speculatable. The latter is important
if we want to recompute intrinsics results instead of communicating them
via memory.

I did use default attributes for almost all readnone attributes but
speculatable only where I had reasonable confidence they cannot
experience UB. That said, someone should double check.

TODO: There seem to be various intrinsics marked Commutative which

should not, e.g., fma and div.

Diff Detail

Event Timeline

jdoerfert created this revision.Sep 17 2021, 11:27 AM
jdoerfert requested review of this revision.Sep 17 2021, 11:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2021, 11:27 AM

@JonChesterfield @arsenm @jhuber6 Something like this should be done for AMDGPU as well in case it wasn't yet.

llvm/include/llvm/IR/IntrinsicsNVVM.td
657

These commutative annotations seem wrong to me. Same for fma.

tra added a comment.Sep 17 2021, 12:06 PM

LGTM in general, modulo the comments below.

llvm/include/llvm/IR/IntrinsicsNVVM.td
657

Agreed.

817

I'm not sure what Commutative attribute would mean for FMA. I guess the multiplicand args are commutative with each other, but the addend is not. I guess the Commutative should ideally list the operands it applies to.

1088–1089

Should this be speculatable, too? It just bitcasts two ints to a double. There are no side effects.

4272

Some of the special registers provide clock and performance monitoring counter info. Speculating those may not be a good idea.

jdoerfert added inline comments.Sep 17 2021, 12:30 PM
llvm/include/llvm/IR/IntrinsicsNVVM.td
817

I'm for removing it here and above.

1088–1089

It escaped my search pattern due to the commutative. Will add speculatable.

4272

As far as I can tell, those are the PTXReadNCSRegIntrinsic_r32 ones below. If any of the IntrNoMem ones would do this, we would CSE them and their usefulness would be rather limited. As long as we allow CSE, I think, all we care about is the potential for UB in case we speculate on them. Reading registers hopefully won't cause UB.

tra accepted this revision.Sep 17 2021, 12:43 PM
tra added inline comments.
llvm/include/llvm/IR/IntrinsicsNVVM.td
817

SGTM.

4272

You're right. These are fine.

This revision is now accepted and ready to land.Sep 17 2021, 12:43 PM

Addresses comments

jdoerfert added inline comments.May 18 2022, 12:16 PM
llvm/include/llvm/IR/IntrinsicsNVVM.td
4284

I think I forgot to make these DefaultAttr too.

Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2022, 12:16 PM