Skip to content

Add a proposal for consistently naming llvm.dx intrinsics. #99

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

bogner
Copy link
Collaborator

@bogner bogner commented Nov 11, 2024

No description provided.

```

Open questions:
- `gstream`, or `stream`?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear this was "gs" (geometry shader) + "stream" for "gsstream", not "gstream". I have a slight preference for "gsstream" to prevent confusion if we were ever to use "stream" in any other context.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me. Updated the heading to spell out geometry shader and removed the open question.

Copy link
Collaborator

@tex3d tex3d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good start (if I do say so myself ;). Should we merge this then file issues for open questions, and update it as we resolve these?

llvm.dx.resource.gathercmp ; (TextureGatherCmp)
llvm.dx.resource.gatherraw ; (TextureGatherRaw)
llvm.dx.resource.atomicbinop ; (AtomicBinOp)
llvm.dx.resource.atomiccmpxchg ; (AtomicCompareExchange)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and pos for position above seem to be the only places we use abbreviations of the dxil op name, maybe better to write it all out everywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these two abbreviations are both defensible:

  • In the "pos" case we're abbreviating from "Texture2DMSGetSamplePosition" to "texturesamplepos". I'm fine with calling this "texturesampleposition" rather than "pos", but it still wouldn't match and I think saying "texture" rather than "texture2dms" is better here.
  • "cmpxchg" is an extremely common abbreviation for "compare and exchange" and easy to parse at a glance, whereas "compareexchange" is arguably harder to read.

I don't feel super strongly here, but I have a weak preference for keeping these as is.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to keep it as-is is good enough for me

@llvm-beanz
Copy link
Collaborator

cc @s-perron & @Keenuts, since we do rely on some of the DX and SPV intrinsics sharing name suffixes.

Copy link
Collaborator

@Keenuts Keenuts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with this!

@bogner bogner merged commit f481ce7 into llvm:main Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants