Skip to content
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

[DirectX][docs] Architecture and design philosophy of DXIL support #78221

Conversation

bogner
Copy link
Contributor

@bogner bogner commented Jan 16, 2024

This documents some of the architectural direction for DXIL and tries
to provide a bit of a map for where to implement different aspects of
DXIL support.

Created using spr 1.3.5-bogner
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 16, 2024

@llvm/pr-subscribers-backend-directx

Author: Justin Bogner (bogner)

Changes

This documents some of the architectural direction for DXIL and tries
to provide a bit of a map for where to implement different aspects of
DXIL support.


Full diff: https://github.com/llvm/llvm-project/pull/78221.diff

2 Files Affected:

  • (added) llvm/docs/DirectX/DXILArchitecture.rst (+102)
  • (modified) llvm/docs/DirectXUsage.rst (+2)
diff --git a/llvm/docs/DirectX/DXILArchitecture.rst b/llvm/docs/DirectX/DXILArchitecture.rst
new file mode 100644
index 00000000000000..9d1edefc3a7686
--- /dev/null
+++ b/llvm/docs/DirectX/DXILArchitecture.rst
@@ -0,0 +1,102 @@
+===============================================
+Architecture and Design of DXIL Support in LLVM
+===============================================
+
+.. contents::
+   :local:
+
+.. toctree::
+   :hidden:
+
+Introduction
+============
+
+LLVM supports reading and writing the `DirectX Intermediate Language.
+<https://github.com/microsoft/DirectXShaderCompiler/blob/main/docs/DXIL.rst>`_,
+or DXIL. DXIL is essentially LLVM 3.7 era bitcode with some
+restrictions and various semantically important operations and
+metadata.
+
+LLVM's implementation philosophy for DXIL support is to treat DXIL as
+merely a representation format as much as possible. When reading DXIL,
+we should translate everyting to generic LLVM constructs when
+possible. Similarly, we should introduce DXIL-specific constructs as
+late as possible in the process of lowering to the format.
+
+There are three places to look for DXIL related code in LLVM: The
+`DirectX` backend, for writing DXIL; The `DXILUpgrade` pass, for
+reading; and in library code that is shared between writing and
+reading. We'll describe these in reverse order.
+
+Common Code for Reading and Writing
+===================================
+
+There's quite a bit of logic that needs to be shared between reading
+and writing DXIL in order to avoid code duplication. While we don't
+have a hard and fast rule about where such code should live, there are
+generally three sensible places. Simple definitions of enums and
+values that must stay fixed to match DXIL's ABI can be found in
+`Support/DXILABI.h`, utilities to translate bidirectionally between
+DXIL and modern LLVM constructs live in `lib/Transforms/Utils`, and
+more analyses that are needed to derive or preserve information are
+implemented as typical `lib/Analysis` passes.
+
+The DXILUpgrade Pass
+====================
+
+Translating DXIL to LLVM IR takes advantage of the fact that DXIL is
+compatible with LLVM 3.7 bitcode, and that modern LLVM is capable of
+"upgrading" older bitcode into modern IR. Simply relying on the
+bitcode upgrade process isn't sufficient though, since that leaves a
+number of DXIL specific constructs around. Thus, we have the
+`DXILUpgrade` pass to transform DXIL operations to LLVM operations and
+smooth over differences in metadata representation.
+
+The DirectX Backend
+===================
+
+The DirectX backend lowers LLVM IR into DXIL. As we're transforming to
+an intermediate format rather than a specific ISA, this backend does
+not follow the instruction selection patterns you might be familiar
+with from other backends. There are two parts to lowering DXIL - a set
+of passes that mutate various constructs into a form that matches how
+DXIL represents those constructs, followed by a limited bitcode
+"downgrader pass".
+
+Before emitting DXIL, the DirectX backend needs to modify the LLVM IR
+such that external operations, types, and metadata is represented in
+the way that DXIL expects. For example, `DXILOpLowering` translates
+intrinsics into `dx.op` calls. It's encouraged to implement parts of
+the DXIL lowering as these kinds of IR to IR passes when possible, as
+that means that they can be easily tested with `opt` and `FileCheck`
+without the need for external tooling.
+
+The second part of DXIL emission is more or less an LLVM bitcode
+downgrader. We need to emit bitcode that matches the LLVM 3.7
+representation. For this, we have `DXILWriter`, which is an alternate
+version of LLVM's `BitcodeWriter`. At present, this is able to
+leverage LLVM's current bitcode libraries to do a lot of the work, but
+it's possible that at some point in the future it will need to be
+completely separate as modern LLVM bitcode evolves.
+
+Testing
+=======
+
+A lot of DXIL testing can be one with typical IR to IR tests using
+`opt` and `FileCheck`, since a lot of the support is implemented in
+terms of IR level passes as described in the previous sections. You
+can see examples of this in `llvm/test/CodeGen/DirectX` as well as
+`llvm/test/Transforms/DXILUpgrade`, and this type of testing should be
+leveraged as much as possible.
+
+However, when it comes to testing the DXIL format itself, IR passes
+are insufficient for testing. For now, the best option we have
+available is using the DXC project's tools in order to round trip.
+These tests are currently found in `test/tools/dxil-dis` and are only
+available if the `LLVM_INCLUDE_DXIL_TESTS` cmake option is set. Note
+that we do not currently have the equivalent testing set up for the
+DXIL reading path.
+
+In the future, we will also want to round trip using the DXIL writing
+and reading paths in order to ensure self consistency and to get test
+coverage when `dxil-dis` isn't available.
diff --git a/llvm/docs/DirectXUsage.rst b/llvm/docs/DirectXUsage.rst
index 18b5c6d02cb00e..79543e19bd34bb 100644
--- a/llvm/docs/DirectXUsage.rst
+++ b/llvm/docs/DirectXUsage.rst
@@ -13,6 +13,8 @@ User Guide for the DirectX Target
 .. toctree::
    :hidden:
 
+   DirectX/DXILArchitecture
+
 Introduction
 ============
 

The DXILUpgrade Pass
====================

Translating DXIL to LLVM IR takes advantage of the fact that DXIL is
Copy link
Contributor

Choose a reason for hiding this comment

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

What will the output of DXILUpgrade Pass look like?
Will it match the input for DirectX backend?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will look like LLVM IR without DXIL specific features. Yes, it should be valid input for the DirectX backend.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will DXILUpgrade Pass share some code with clang codeGen since they both generate input for DirectX backend?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, of course. I added a short paragraph mentioning how this works to try to clarify.

Co-authored-by: S. Bharadwaj Yadavalli <Bharadwaj.Yadavalli@microsoft.com>
llvm/docs/DirectX/DXILArchitecture.rst Outdated Show resolved Hide resolved

The second part of DXIL emission is more or less an LLVM bitcode
downgrader. We need to emit bitcode that matches the LLVM 3.7
representation. For this, we have `DXILWriter`, which is an alternate
Copy link
Contributor

Choose a reason for hiding this comment

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

Just from a naming perspective it would be nice to have a DXILReader to match the DXILWriter, but it looks like we have DXILUpgrade instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The nuance here is that we don't actually implement a DXILReader at all. As per the explanation of DXILUpgrade earlier in the doc:

Translating DXIL to LLVM IR takes advantage of the fact that DXIL is
compatible with LLVM 3.7 bitcode, and that modern LLVM is capable of
"upgrading" older bitcode into modern IR. Simply relying on the
bitcode upgrade process isn't sufficient though, ...

This is really the rationale for the somewhat unusual naming of DXILUpgrade, since it isn't the reader and it basically operates as an extension of LLVM's standard bitcode upgrade path. Let me know if there's some wording missing that could help clarify that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it makes sense that's what its doing, but that is also a bit of an implementation detail. I could also see us calling the Writer a "Downgrader" instead. In fact, that's what the doc says it does!

The second part of DXIL emission is more or less an LLVM bitcode
downgrader.

It's not a big deal, but I think having a symmetric name for reader/writer or upgrader/downgrader would be nice to quickly understand that the two operations are inverses and how you would do a roundtrip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit reticent to use a symmetric name here since they really aren't inverses on their own.

  • The "downgrading" aspects of writing DXIL largely happen before we get to DXILWriter (which just writes the format), and it should indeed be possible to run the backend up to but not including DXILWriter, and then round trip that with DXILUpgrade.
  • Similarly, DXILUpgrade does not read bitcode at all (let alone 3.7 era bitcode) - the standard LLVM bitcode reading/upgrade path gives us IR that's suitable input for DXILUpgrade, which then goes ahead and upgrades the DXIL specific constructs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added sentences to the DXILUpgrade section and to the paragraph about lowering/downgrading to try to clarify this

that we do not currently have the equivalent testing set up for the
DXIL reading path.

In the future, we will also want to round trip using the DXIL writing
Copy link
Contributor

Choose a reason for hiding this comment

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

"In the future"....is this planned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is crucial for sufficient testing of the DirectX backend as we make more progress, especially since most bots are unlikely to have the external dxil-dis available. I say "in the future" here for now since it isn't actually practical to do this until a bit more of the implementation is complete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reworded this to "As soon as we are able"

it's possible that at some point in the future it will need to be
completely separate as modern LLVM bitcode evolves.

Testing
Copy link

Choose a reason for hiding this comment

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

Not sure if we want to call it out this early, but I can see value in eventually adding two additional testing methodologies:

  1. Near term - compare output of DXC to LLVM. Might be best as an external source of testing, but, given the same HLSL, the DXIL should be functionally equivalent. Although, ideally, a one-to-one match would be best, I don't think it's possible.
    Without a DXIL interpreter, this is probably too fragile and is probably better to have....
  2. Execution testing. We need to be able to run the DXIL through IHV backends (or WARP) to guarantee correctness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with all of this but I couldn't come up with much concrete to say for now

Copy link

Choose a reason for hiding this comment

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

I just honestly can't see how we can have any confidence that the DXIL backend is working until there's functional tests using the generated code. Unit tests can only take us so far, but it won't be long before that isn't going to be sufficient.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't want to in any way diminish the importance of execution testing (because it is very important), but I know that both @bogner, and myself have worked on backend implementations for hardware that didn't yet exist and the development and testing strategies that are common in LLVM have allowed us to do so with very high levels of confidence in the compiler implementation.

For the purposes of this document I think it is valuable to focus on compiler unit testing and leave execution testing as a separate problem because it is a large and complicated problem that needs its own separate design. The approach we currently take in DXC is too limited for the goals of our Clang implementation so we'll really need to come up with something much more ambitious (although it certainly can draw inspiration and experience from what we did in DXC).

Created using spr 1.3.5-bogner
Created using spr 1.3.5-bogner
Created using spr 1.3.5-bogner
@bogner bogner merged commit 151559c into main Jan 31, 2024
4 of 5 checks passed
@bogner bogner deleted the users/bogner/sprdirectxdocs-architecture-and-design-philosophy-of-dxil-support branch January 31, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants