This is an archive of the discontinued LLVM Phabricator instance.

[NFC] [HLSL] Move common metadata to LLVMFrontend
ClosedPublic

Authored by beanz on Oct 3 2022, 3:03 PM.

Details

Summary

This change pulls some code from the DirectX backend into a new
LLVMFrontendHLSL library to share utility data structures between the
HLSL code generation in Clang and the backend in LLVM.

This is a small refactoring as a first start to get code into the
right structure and get the library built and dependencies correct.

Fixes #58000 (https://github.com/llvm/llvm-project/issues/58000)

Diff Detail

Unit TestsFailed

Event Timeline

beanz created this revision.Oct 3 2022, 3:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2022, 3:03 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
beanz requested review of this revision.Oct 3 2022, 3:03 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 3 2022, 3:03 PM
python3kgae added inline comments.Oct 3 2022, 4:23 PM
llvm/include/llvm/Frontend/HLSL/HLSLResource.h
36

Could we save things like shape and component type instead of a StringRef which needs to be parsed later?

The OpenMP frontend is mainly an IRBuilder. It is a different layering than for HLSL. Are there plans for an HLSL(IR)Builder?

beanz added a comment.Oct 4 2022, 9:35 AM

The OpenMP frontend is mainly an IRBuilder. It is a different layering than for HLSL. Are there plans for an HLSL(IR)Builder?

HLSL and OpenMP are different in a lot of ways. HLSL's code generation is fairly consistent with C/C++. There will be some areas where we have special IR generation mechanics (like around copy-in/copy-out function parameters), which I expect we'll create some sort of reusable API for. I'm unsure if we have enough unique cases to make a full builder worth it.

Similar to OpenCL we also have a lot of metadata that needs to get passed from the frontend to the backend. That's really where this patch is starting. Having shared utilities for reading and writing metadata.

llvm/include/llvm/Frontend/HLSL/HLSLResource.h
36

This change is an NFC refactoring to move code around and start sharing between the front-end and backend. We have a separate issue tracking extending the frontend attribute support to capture more metadata (https://github.com/llvm/llvm-project/issues/57991). That will be a separate change.

beanz updated this revision to Diff 467606.Oct 13 2022, 2:28 PM

Rebasing

python3kgae added inline comments.Oct 13 2022, 2:33 PM
llvm/lib/Target/DirectX/DXILResource.h
46

Could we move ResourceBase::Kinds to Frontend/HLSL/HLSLResource.h so we can share it between clang and llvm?

beanz added inline comments.Oct 13 2022, 7:29 PM
llvm/lib/Target/DirectX/DXILResource.h
46

Within reason, we can move any code that makes sense to share. We should do that in changes where the moved code will be used rather than making this change larger.

python3kgae accepted this revision.Oct 13 2022, 9:13 PM
This revision is now accepted and ready to land.Oct 13 2022, 9:13 PM
This revision was landed with ongoing or failed builds.Oct 14 2022, 11:45 AM
This revision was automatically updated to reflect the committed changes.

I don't quite know why, but it seems like this new library breaks tests of llvm-config; if I start out with an empty build directory and run ninja check-llvm (or ninja llvm-test-depends), then this library doesn't get built, and llvm-config prints llvm-config: error: missing: $HOME/code/llvm-project/llvm/build/lib/libLLVMFrontendHLSL.a - and that breaks tools/llvm-config/booleans.test and tools/llvm-config/system-libs.test.

I don't quite know why, but it seems like this new library breaks tests of llvm-config; if I start out with an empty build directory and run ninja check-llvm (or ninja llvm-test-depends), then this library doesn't get built, and llvm-config prints llvm-config: error: missing: $HOME/code/llvm-project/llvm/build/lib/libLLVMFrontendHLSL.a - and that breaks tools/llvm-config/booleans.test and tools/llvm-config/system-libs.test.

I see the same issue.

I think that the reason we don't see the same failure for e.g. OpenACC is that there's a unittest/Frontend that requires OpenACC - if I comment out that subdir in the unittests/CMakeLists.txt it fails llvm-config test in the same way for OpenACC. Similarly, if I add FrontendHLSL as an LLVM_LINK_COMPONENT in unittests/Frontend/CMakeLists.txt (even though it isn't), the static lib is generated and llvm-lit llvm-config test passes.

beanz added a comment.Oct 18 2022, 6:37 AM

I think I have a fix for this and I'll get it up today.

beanz added a comment.Oct 18 2022, 8:05 AM

The llvm-config test issue should be resolved in rGa4b010034f57