[PATCH] D122627: [HLSL] Fix MSFT Attribute parsing, add numthreads
pow2clk added a comment.
Looks good. I made a comment about the CS 4.0 thing, but that can be addressed
when more thorough examination of the requested shader models comes around.
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6857
+ uint32_t ThreadMax = 1024;
+ if (SMVersion.getMajor() <= 4) {
+ZMax = 1;
As we discussed, compute shaders didn't actually exist pre-4.0 and I don't know
that we'll ever seek to do anything with a 4.0 shader model except convert it
to a higher version and issue a notification, but that handling is clearly not
the intent of this change. So it's fine with me.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122627/new/
https://reviews.llvm.org/D122627
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128845: [HLSL]Add -O and -Od option for dxc mode.
pow2clk added a comment. Looks good apart from my uncertainty as to what -fcgl should or shouldn't imply Comment at: clang/test/Driver/dxc_O.hlsl:9 +// CHECK: "-O0" +// CHECK-SAME: "-dxc-opt-disable" + It looks to me that these are added when -Od is specified, not -fcgl (aka pristine-llvm I guess ;)) Is the comment wrong or should these be added as part of pristine-llvm too? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128845/new/ https://reviews.llvm.org/D128845 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128462: [HLSL] add -I option for dxc mode.
pow2clk added a comment. I'm confused by the ways this option is allowed to be specified Comment at: clang/include/clang/Driver/Options.td:6824 +class DXCJoinedOrSeparateConflict : Option<["--", "/", "-"], name, + KIND_JOINED_OR_SEPARATE>, Group, Flags<[DXCOption, NoXarchOption]>; I'm not sure why this has the suffix "Conflict" In addition, you can't currently specify the "I" option with "--" in DXC. Comment at: clang/test/Driver/dxc_I.hlsl:1 +// RUN: %clang_dxc -I test.h -### %s 2>&1 | FileCheck %s + test.h is kind of a funny name for a directory. 🤔 Maybe just call it "test"? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128462/new/ https://reviews.llvm.org/D128462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D125655: [HLSL] add -P option for dxc mode.
pow2clk added a comment. Mostly just some comments on user output Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:684 +def warn_drv_dxc_ignore_output_for_preprocess : Warning< + "output compiler options like -Fo ignored with Preprocess">, + InGroup; As I read it, "like -Fo" means either "-Fo" or "-o". I think saying that would be less vague. Comment at: clang/include/clang/Driver/Options.td:6793 def fcgl : DXCFlag<"fcgl">, Alias; +def dxc_P : Option<["--", "/", "-"], "P", KIND_SEPARATE>, + Group, Flags<[DXCOption, NoXarchOption]>, --P is not allowed by DXC Comment at: clang/include/clang/Driver/Options.td:6796 + HelpText<"Preprocess to file (must be used alone)." + "Same as -E + -o.">; It's not the same as DXC's "-E", which is an unfortunate clash since I love redirecting preprocessed output into less 😒 I realize that we are using some of the clang defined flags to represent the weird DXC ones and in this case that is -E and -o, but this text to the user isn't going to be too helpful to the user who only has access to the DXC -E. I'm not sure what "used alone" means in this context as you can very much include other flags alongside it Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3480 + options::OPT_dxil_validator_version, options::OPT_E, options::OPT_S, + options::OPT_emit_llvm, options::OPT_disable_llvm_passes}; I think we need OPT_o here too in order to capture the full intent since -E doesn't include an output file Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125655/new/ https://reviews.llvm.org/D125655 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D124751: [HLSL] Support -E option for HLSL.
pow2clk added a comment. Looks to me that all the comments were responded to. I see nothing else worth commenting on. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124751/new/ https://reviews.llvm.org/D124751 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D132711: [HLSL] add sqrt library function
pow2clk added a comment. A very minor suggestion. This looks pretty straightforward. Comment at: clang/test/CodeGenHLSL/builtins/sqrt.hlsl:6 +// RUN: dxil-pc-shadermodel6.3-library %s -emit-llvm -disable-llvm-passes \ +// RUN: -o - | FileCheck %s --check-prefix=NO_HALF + The -fnative-half-type suggests that you're readying this for when 16-bit types are available. That's fine, but in the spirit of starting as early as possible as we tend to do with these, you might want to target shadermodel6.2 instead since that's where they were introduced and sqrt is as old as HLSL 1.0 AFAIK. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132711/new/ https://reviews.llvm.org/D132711 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
