This is an archive of the discontinued LLVM Phabricator instance.

[HLSL] Add utility to convert environment to stage
ClosedPublic

Authored by beanz on Oct 10 2022, 8:53 AM.

Details

Summary

We had a bunch of places in the code where we were translating triple
environment enum cases to shader stage enum cases. The order of these
enums needs to be kept in sync for the translation to be simple, but we
were not properly handling out-of-bounds cases.

In normal compilation out-of-bounds cases shouldn't be possible because
the driver errors if you don't have a valid shader environment set, but
in clang tooling that error doesn't get treated as fatal and parsing
continues. This can result in crashes in clang tooling for out-of-range
shader stages.

To address this, this patch adds a constexpr method to handle the
conversion which handles out-of-range values by converting them to
Invalid.

Since this new method is a constexpr, the tests for this are a group of
static_asserts in the implementation file that verifies the correct
conversion for each valid enum case and validates that other cases are
converted to Invalid.

Diff Detail

Event Timeline

beanz created this revision.Oct 10 2022, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2022, 8:53 AM
beanz requested review of this revision.Oct 10 2022, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2022, 8:53 AM

Backend needs the same thing.
Is it possible to move this to llvm and share it between frontend and backend?

beanz added a comment.Oct 10 2022, 9:33 AM

Backend needs the same thing.
Is it possible to move this to llvm and share it between frontend and backend?

This translates the triple to a clang-defined enum... so strictly speaking no, that isn't possible.

Can you elaborate on the context where the backend needs this? I suspect in the backend we need a similar but not quite the same utility. Specifically, I think we need to translate the environment value to an unsigned integer. For all cases where an enum makes sense we should be using the Environment value in the backend.

beanz added a comment.Oct 10 2022, 9:38 AM

Worth noting, we do have a similar set of static_asserts in Triple.cpp to validate the ordering of enum cases and that the subtraction results in the appropriate values:

https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/Triple.cpp#L1942

Backend needs the same thing.
Is it possible to move this to llvm and share it between frontend and backend?

This translates the triple to a clang-defined enum... so strictly speaking no, that isn't possible.

Can you elaborate on the context where the backend needs this? I suspect in the backend we need a similar but not quite the same utility. Specifically, I think we need to translate the environment value to an unsigned integer. For all cases where an enum makes sense we should be using the Environment value in the backend.

I think most use cases could be covered by switch and make default go llvm_unreachable.
It would be helpful to have util functions like isGraphics (which return true for graphics shader like vs/ps), isRay( which return true on ray tracing entry functions).

beanz added a comment.Oct 10 2022, 4:58 PM

We should absolutely add utility functions when we have uses for them.

bogner accepted this revision.Oct 12 2022, 12:25 PM
bogner added a subscriber: bogner.

Even without the tooling aspect the addition of the static asserts is a strict improvement. looks good, thanks!

clang/include/clang/Basic/HLSLRuntime.h
39

You're not actually introducing the dependency here (it was already there), but neither ShaderStage in LangOptions.h nor the shader stage EnvironmentType in Triple.h mention that they need to be kept in sync with the other. Can you add comments to both that note the relationship?

This revision is now accepted and ready to land.Oct 12 2022, 12:25 PM
beanz added inline comments.Oct 12 2022, 1:44 PM
clang/include/clang/Basic/HLSLRuntime.h
39

Will do!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2022, 2:31 PM