This is an archive of the discontinued LLVM Phabricator instance.

Add DXIL Bitcode Writer and DXIL testing
ClosedPublic

Authored by beanz on Mar 19 2022, 1:26 PM.

Details

Summary

This change is a big blob of code that isn't easy to break up. It
either comes in all together as a blob, works and has tests, or it
doesn't do anything.

Logically you can think of this patch as three things:
(1) Adding virtual interfaces so the bitcode writer can be overridden
(2) Adding a new bitcode writer implementation for DXIL
(3) Adding some (optional) crazy CMake goop to build the
DirectXShaderCompiler's llvm-dis as dxil-dis for testing

Diff Detail

Event Timeline

beanz created this revision.Mar 19 2022, 1:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2022, 1:26 PM
beanz requested review of this revision.Mar 19 2022, 1:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2022, 1:26 PM
nikic requested changes to this revision.Mar 19 2022, 1:55 PM
nikic added a subscriber: nikic.

This implementation is not compatible with opaque pointers (https://llvm.org/docs/OpaquePointers.html) and will likely need a significant rework to support them.

This revision now requires changes to proceed.Mar 19 2022, 1:55 PM
beanz added a comment.Mar 19 2022, 5:37 PM

This implementation is not compatible with opaque pointers (https://llvm.org/docs/OpaquePointers.html) and will likely need a significant rework to support them.

I don’t disagree, but…

Much of this code will work just fine with opaque pointers, and opaque pointers aren’t fully supported in clang (the only frontend we’re trying to support).

Since this code is going into an experimental backend, does opaque pointer support need to block this from landing, or can that be addressed subsequently?

pete added a comment.Mar 19 2022, 8:25 PM

Overall LGTM. I don’t have a strong opinion on opaque pointers so nothing to add there.

llvm/test/tools/dxil-dis/FMA.ll
7

Looks like this is actually testing function attributes, not fma. Can you change the test name, or add the check line for an fma if that is the intention?

llvm/tools/dxil-dis/CMakeLists.txt
2

Is dxil-dis only a testing tool, or is it shipped in a distribution too?

If it’s only for tests then I think this is fine. If it’s shipped, then I wonder if the cmake variable should drop “TESTS”. Up to you though.

See https://discourse.llvm.org/t/opaque-pointers-status-update/60296 for the current status of opaque pointers.

llvm/include/llvm/Bitcode/BitcodeWriter.h
37 ↗(On Diff #416726)

Instead of returning a unique_ptr to an object that has exactly one function called on it, can you just pass in a callback function that constructs the writer and calls write()?

38 ↗(On Diff #416726)

Please land the whitespace change here separately.

llvm/lib/Bitcode/Writer/BitcodeWriterPass.cpp
1 ↗(On Diff #416726)

BitcodeWriterPass is a tiny amount of code... you might as well just fork it. (Normally, I'd be in favor of trying to share more code, but I'd prefer to minimize the changes to target-independent code for now.)

nikic added a comment.Mar 20 2022, 2:02 AM

This implementation is not compatible with opaque pointers (https://llvm.org/docs/OpaquePointers.html) and will likely need a significant rework to support them.

I don’t disagree, but…

Much of this code will work just fine with opaque pointers, and opaque pointers aren’t fully supported in clang (the only frontend we’re trying to support).

Since this code is going into an experimental backend, does opaque pointer support need to block this from landing, or can that be addressed subsequently?

Yes, opaque pointer support is a hard requirement for any new code introduced in LLVM. Of course, it's okay if you implement it in a separate patch from this one, it just needs to be part of the patch stack and land at the same time.

Yes, opaque pointer support is a hard requirement for any new code introduced in LLVM.

Is this info documented somewhere with "roadmap" or "future plans"? Like telling developers / users "we are going to ditch undef, we are going to switch to opaque ptrs, etc.." . It would be nice addition to have such place with high level plans for future.

beanz added a comment.Mar 20 2022, 5:33 PM

Yes, opaque pointer support is a hard requirement for any new code introduced in LLVM.

Not to be difficult, but was this discussed somewhere that I missed? I didn’t see it in any of the status updates or project plans for opaque pointers. I have no objection to doing the work (I’ve already started). I actually don’t even mind not merging this patch until I have it working. The opaque pointer effort will actually be a net win for the larger project I’m working on, so kudos to that :D.

I greatly dislike “you can’t land <x> because of <not done thing>” being a requirement _ever_ in LLVM, but especially in cases where the not completed work doesn’t have a final completion target (as the latest status update on opaque pointers says).

We held up a lot of great work behind the “new” pass manager. For _years_ nobody was allowed to add features to the “legacy” pass manager or even fix infrastructure issues in ways that relied on the pass manager.

Drawing hard lines in the sand like this is fine if there is wide community acceptance (as with any coding standard), but nobody should be able to draw these lines without discussion and agreement.

Yes, opaque pointer support is a hard requirement for any new code introduced in LLVM.

LLVM tends to document a lot. Why is there no roadmap document? Get rid of old pass manager, move to globalisel, move to opaque pointers, ...

xbolva00 added a subscriber: tonic.Mar 21 2022, 1:49 AM

Yes, opaque pointer support is a hard requirement for any new code introduced in LLVM.

LLVM tends to document a lot. Why is there no roadmap document? Get rid of old pass manager, move to globalisel, move to opaque pointers, ...

Can LLVM board create one?

@tstellar @tonic

nikic added a comment.Mar 21 2022, 2:55 AM

@xbolva00 Don't think the board has any relation to this, but I do agree that having some central documentation would be great. While many of the ongoing migration efforts are well-documented by themselves, there's currently no central place that collects them. I've created https://reviews.llvm.org/D122122 as a starting point, feedback welcome.

beanz added a comment.Mar 21 2022, 2:02 PM

@xbolva00 Don't think the board has any relation to this, but I do agree that having some central documentation would be great. While many of the ongoing migration efforts are well-documented by themselves, there's currently no central place that collects them. I've created https://reviews.llvm.org/D122122 as a starting point, feedback welcome.

@nikic, even that document doesn't say support for opaque pointers is a "hard requirement" for new contributions.

Given that there is no documentation or clear community consensus anywhere that I can find that it should be a hard requirement, and that this code is going into an experimental backend, which (as documented in the support policy: https://llvm.org/docs/SupportPolicy.html) means "you break it, I fix it". Would you revise your "needs revision" feedback?

nikic added a comment.Mar 21 2022, 3:03 PM

Yes, opaque pointer support is a hard requirement for any new code introduced in LLVM.

Not to be difficult, but was this discussed somewhere that I missed? I didn’t see it in any of the status updates or project plans for opaque pointers. I have no objection to doing the work (I’ve already started). I actually don’t even mind not merging this patch until I have it working. The opaque pointer effort will actually be a net win for the larger project I’m working on, so kudos to that :D.

I greatly dislike “you can’t land <x> because of <not done thing>” being a requirement _ever_ in LLVM, but especially in cases where the not completed work doesn’t have a final completion target (as the latest status update on opaque pointers says).

We held up a lot of great work behind the “new” pass manager. For _years_ nobody was allowed to add features to the “legacy” pass manager or even fix infrastructure issues in ways that relied on the pass manager.

Drawing hard lines in the sand like this is fine if there is wide community acceptance (as with any coding standard), but nobody should be able to draw these lines without discussion and agreement.

I'm not sure what you want to hear here. The opaque pointers migration was decided many years ago, long before I started working on LLVM. Well-informed reviewers have been rejecting patches violating opaque pointers principles years before LLVM even had a proper opaque pointer type. Of course, enforcement on this has been inconsistent and somewhat dependent on which reviewer you hit.

Now that the opaque pointers migration is close to complete I'm keeping an active eye out to make sure there are no regressions. LLVM itself is essentially clean under opaque pointers, and we're maybe two weeks away from proposing a default switch in Clang. Which is why I'm so surprised that literally the first time I'm seeing push-back against an opaque pointer requirement in a review is now.

I think that for this change in particular it is important to resolve this issue in advance, because it may impact your overall design in a non-trivial way. Your current handling of DXIL is predicated on LLVM IR and DXIL being essentially the same, minus a few cosmetic differences like fneg vs fsub. With opaque pointers, LLVM IR and DXIL will differ in a more substantial way, and I'd naively expect this to have some impact on your implementation approach.

Finally, if you want to make a new pass manager analogy, I'm not telling you that you can't work on the legacy pass manager, I'm only telling you that you should implement functionality for both the legacy and new pass manager at the same time. That seems like a pretty reasonable request to me? In fact, our biggest failures in the NewPM migration were related to cases where this principle was not followed, and changes were made only to either the new or the legacy pass manager. Let's avoid that to the degree it is possible.

@xbolva00 Don't think the board has any relation to this, but I do agree that having some central documentation would be great. While many of the ongoing migration efforts are well-documented by themselves, there's currently no central place that collects them. I've created https://reviews.llvm.org/D122122 as a starting point, feedback welcome.

@nikic, even that document doesn't say support for opaque pointers is a "hard requirement" for new contributions.

Given that there is no documentation or clear community consensus anywhere that I can find that it should be a hard requirement, and that this code is going into an experimental backend, which (as documented in the support policy: https://llvm.org/docs/SupportPolicy.html) means "you break it, I fix it". Would you revise your "needs revision" feedback?

It would be great if there were more documentation around the migration timelines, but lack of documentation does not mean lack of plans or lack of consensus. I would recommend starting a "Does the DXIL backend need to be compatible with Opaque Pointers" discussion on Discourse just so we can open this question up to a wider audience and get a little more clarity on this issue.

It would be great if there were more documentation around the migration timelines, but lack of documentation does not mean lack of plans or lack of consensus. I would recommend starting a "Does the DXIL backend need to be compatible with Opaque Pointers" discussion on Discourse just so we can open this question up to a wider audience and get a little more clarity on this issue.

I've posted patches already to get the DIXL emitter working with opaque pointers. It still relies on typed pointers being a thing (not in the IR, but constructable llvm::Types). I'm also working on removing that reliance, and expect to have patches up this week.

rnk added a comment.Mar 23 2022, 8:19 PM

I've posted patches already to get the DIXL emitter working with opaque pointers. It still relies on typed pointers being a thing (not in the IR, but constructable llvm::Types). I'm also working on removing that reliance, and expect to have patches up this week.

Glad to hear it. That's a key thing I wanted to bring up. Right now, an opaque pointer is basically just a pointer with a null element type, but I think the long term vision is that we remove the field altogether. I think we're a long way from here to there, but we should plan on that as the end state.

MaskRay added inline comments.Mar 23 2022, 10:41 PM
llvm/lib/Target/DirectX/DXILBitcodeWriter.cpp
106 ↗(On Diff #416726)

https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
Don’t duplicate function or class name at the beginning of the comment.

166 ↗(On Diff #416726)

range-based for

1689 ↗(On Diff #416726)

The FIXME (copied from BitcodeWriter.cpp) is probably unrelated and should be deleted.

BitcodeWriter.cpp seems to (ab)use 64 inline elements a lot. Does DXIL need 64?
You may consider SmallVector<unsigned>.

1700 ↗(On Diff #416726)

ranged-based for

1702 ↗(On Diff #416726)
1705 ↗(On Diff #416726)
1711 ↗(On Diff #416726)

Just remove #if 0. If you need comments, use //

1878 ↗(On Diff #416726)

These llvm_unreachable calls should be assert

2092 ↗(On Diff #416726)

range-based for

llvm/tools/dxil-dis/CMakeLists.txt
9

LLVM_TARGETS_TO_BUILD is a superset. I think with this change this line wont' need a change when DirectX becomes non-experimental.

40

Any reason cmake/modules/AddLLVM.cmake add_llvm_tool_symlink can't be used?

beanz updated this revision to Diff 418303.Mar 25 2022, 12:49 PM

Updating based on review feedback.

This change forks the required infrastrcuture from the LLVM Bitcode writer into the DirectX backend instead of relying on refactoring of the existing pieces to be reusable.

The last update makes this change a lot bigger because it is forking a bunch of code out of the bit writer library as per review feedback.

beanz added a comment.Apr 5 2022, 8:10 AM

@nikic, are you satisfied with the additional patches in the stack which support opaque pointers?

nikic added a comment.Apr 6 2022, 8:00 AM

@nikic, are you satisfied with the additional patches in the stack which support opaque pointers?

I've only reviewed the first patch in the stack (D122268) yet, but have two high level questions:

  1. How does/will this handle constant expressions?
  2. How is dxil-dis supposed to work with opaque pointers?
beanz added a comment.Apr 6 2022, 9:32 AM

@nikic, are you satisfied with the additional patches in the stack which support opaque pointers?

I've only reviewed the first patch in the stack (D122268) yet, but have two high level questions:

  1. How does/will this handle constant expressions?

There are some gaps in the type analysis (constant expressions being one), but the method used here should apply. By mapping Values to typed pointer types, I can emit the typed pointer type in place of the opaque pointer type in the bitstream.

  1. How is dxil-dis supposed to work with opaque pointers?

dxil-dis is LLVM 3.7's llvm-dis. It _can't_ handle opaque pointers, and doesn't need to. The references to the opaque pointer type are replaced with typed pointer types that are encoded matching LLVM 3.7's PointerType.

nikic added a comment.Apr 6 2022, 10:08 AM

There are some gaps in the type analysis (constant expressions being one), but the method used here should apply. By mapping Values to typed pointer types, I can emit the typed pointer type in place of the opaque pointer type in the bitstream.

The bit I'm specifically concerned about is that you can't insert bitcasts in the prepare pass, as those would always get folded away. But I guess you can still materialize them directly during bitcode writing.

  1. How is dxil-dis supposed to work with opaque pointers?

dxil-dis is LLVM 3.7's llvm-dis. It _can't_ handle opaque pointers, and doesn't need to. The references to the opaque pointer type are replaced with typed pointer types that are encoded matching LLVM 3.7's PointerType.

Oh, sorry, that was confusion on my part. Somehow I thought that dxil-dis is just a rebranded llvm-dis, I didn't realize that it's built from an external repo.

beanz added a comment.Apr 6 2022, 10:11 AM

The bit I'm specifically concerned about is that you can't insert bitcasts in the prepare pass, as those would always get folded away. But I guess you can still materialize them directly during bitcode writing.

We only need to insert those because of how instructions are numbered in the bitcode writer. I don't _think_ we will need them in the ConstantExprs, but I may not be fully thinking that through. If we needed to materialize them in the bitwriter we can do that (especially now that we're forking the whole thing).

beanz added inline comments.Apr 13 2022, 6:33 PM
llvm/tools/dxil-dis/CMakeLists.txt
40

Sorry @MaskRay, I should have acknowledged this comment earlier. add_llvm_tool_symlink uses generator expressions to read values from the underlying target that doesn't work with this terrible external project goop.

nikic accepted this revision.Apr 14 2022, 12:29 PM

Removing me as a blocker here, I think the opaque pointers concern is sufficiently addressed by dependent patches. I haven't looked carefully over the code here, on the assumption that this is just boring "copy of bitcode writing with necessary tweaks for DXIL".

llvm/test/tools/dxil-dis/FMA.ll
7

Comment not resolved.

This revision is now accepted and ready to land.Apr 14 2022, 12:29 PM
This revision was landed with ongoing or failed builds.Apr 15 2022, 4:50 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Apr 16 2022, 9:58 AM
thakis added inline comments.
llvm/tools/dxil-dis/CMakeLists.txt
29

Adding a dep to another github repo is pretty unusual, even though it's off by default. Was this part explicitly discussed somewhere?

beanz added inline comments.Apr 16 2022, 11:32 AM
llvm/tools/dxil-dis/CMakeLists.txt
29

Not explicitly. The dependency is optional and only for testing. It builds a copy of the modified llvm-3.7 llvm-dis tool to verify that the newly emitted Bitcode can be read.

Each part of this is configurable. It can clone and build, or build from a local directory, or just take a prebuilt tool.

llvm/lib/Target/DirectX/DXILWriter/DXILBitcodeWriter.h