[PATCH] D122082: Add DXIL Bitcode Writer and DXIL testing

[Pete Cooper via Phabricator via llvm-commits]

pete added a comment.

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



================
Comment at: llvm/test/tools/dxil-dis/FMA.ll:6
+
+; CHECK: Function Attrs: nounwind readnone
+; Function Attrs: norecurse nounwind readnone willreturn
----------------
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?


================
Comment at: llvm/tools/dxil-dis/CMakeLists.txt:1
+option(LLVM_INCLUDE_DXIL_TESTS "Include DXIL tests" Off)
+mark_as_advanced(LLVM_INCLUDE_DXIL_TESTS)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122082