[PATCH] D127166: [DirectX] Add DirectX target object writer

Fri Jun 17 11:09:06 PDT 2022

beanz added inline comments.

================
Comment at: llvm/test/CodeGen/DirectX/embed-dxil.ll:3
 ; RUN: opt %s -dxil-embed -S -o - | FileCheck %s
-target triple = "dxil-unknown-unknown"
+; RUN: llc %s --filetype=obj -o - | obj2yaml | FileCheck %s --check-prefix=DXC
+target triple = "dxil-unknown-shadermodel6.5-compute"
----------------
python3kgae wrote:
> beanz wrote:
> > beanz wrote:
> > > python3kgae wrote:
> > > > Is it possible to extract the dxil part out of the container and run dxil-dis to test the output dxil?
> > > There are a few reasons _not_ to use dxil-dis here.
> > > 
> > > First and foremost, this test is testing the structure of the generated DXContainer file, not the dxil encoding itself. In that regard dxil-dis doesn't help because while it can extract the bitcode from a DXContainer, it doesn't actually dump any of the structural information about the container itself (which is what is important here).
> > > 
> > > Additionally dxil-dis is an optional external dependency. Tests that rely on dxil-dis only run if you have dxil-dis available, and I don't expect most LLVM developers will have dxil-dis available. Maximizing test coverage while not depending on external dependencies is a big win.
> > Also worth noting here: with this change llc with -filetype=obj will produce DXContainer files when targeting DXIL. This will result in all the existing dxil-dis tests that use llc to operate on DXContainers instead of bitcode files. The tests all work as long as you have a reasonably up-to-date dxil-dis because that feature was added to dxil-dis recently.
> Then could we have a separate dxil-dis test in dxil-dis folder to make sure the whole pipeline works?
We don't need to add tests for that. The existing dxil-dis tests will already provide that coverage.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127166/new/

https://reviews.llvm.org/D127166