-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[HLSL] Change clang Driver Options to not set CXXOperatorNames #126758
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like the wrong place to put this test. We don't need actual codegen here, we just need to make sure it parses.
I'd put this under SemaHLSL and instead use a run line like:
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library %s -ast-dump | FileCheck %s
Restricting test cases to only running the parts of the compiler that are strictly required to verify correctness of a change reduces the time it takes to run the test. While in isolation for a single test that may only be shaving off fractions of a second, in aggregate across the entire test suite it can have a huge impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok to keep the tests as check-label? Tests are still passing with Label? Or should this change to // CHECK: FunctionDecl {{.*}} NAME void ()
Which is how it seems other tests are setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CHECK-LABEL
doesn't actually change how the matching is performed (see: https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-label-directive). Notably:
It is treated identically to a normal CHECK directive except that FileCheck makes an additional assumption that a line matched by the directive cannot also be matched by any other check present in
match-filename
;...
Whether you use CHECK
or CHECK-LABEL
here doesn't really matter, but the strings you are matching on are too simple, you should be verifying that they are FunctionDecl
s not just that the string and
appears on a line.
@llvm/pr-subscribers-hlsl @llvm/pr-subscribers-clang Author: Farzon Lotfi (farzonl) Changes
Full diff: https://github.com/llvm/llvm-project/pull/126758.diff 2 Files Affected:
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 1cf62ab46613456..618815db2843404 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -3397,7 +3397,7 @@ def fno_objc_weak : Flag<["-"], "fno-objc-weak">, Group<f_Group>,
def fno_omit_frame_pointer : Flag<["-"], "fno-omit-frame-pointer">, Group<f_Group>,
Visibility<[ClangOption, FlangOption]>;
defm operator_names : BoolFOption<"operator-names",
- LangOpts<"CXXOperatorNames">, Default<cplusplus.KeyPath>,
+ LangOpts<"CXXOperatorNames">, Default<!strconcat(cplusplus.KeyPath, " && !",hlsl.KeyPath)>,
NegFlag<SetFalse, [], [ClangOption, CC1Option],
"Do not treat C++ operator name keywords as synonyms for operators">,
PosFlag<SetTrue>>;
diff --git a/clang/test/CodeGenHLSL/use-cxx-alt-operator-names.hlsl b/clang/test/CodeGenHLSL/use-cxx-alt-operator-names.hlsl
new file mode 100644
index 000000000000000..8ae253a9f3c06cb
--- /dev/null
+++ b/clang/test/CodeGenHLSL/use-cxx-alt-operator-names.hlsl
@@ -0,0 +1,41 @@
+
+// RUN: %clang_cc1 -finclude-default-header -x hlsl -triple \
+// RUN: dxil-pc-shadermodel6.3-library %s \
+// RUN: -emit-llvm -disable-llvm-passes -o - | FileCheck %s
+
+// RUN: %clang_cc1 -finclude-default-header -x hlsl -triple \
+// RUN: spirv-unknown-vulkan-compute %s \
+// RUN: -emit-llvm -disable-llvm-passes -o - | FileCheck %s
+
+// CHECK-LABEL: and
+void and() {}
+
+// CHECK-LABEL: and_eq
+void and_eq() {}
+
+// CHECK-LABEL: bitand
+void bitand() {}
+
+// CHECK-LABEL: bitor
+void bitor() {}
+
+// CHECK-LABEL: compl
+void compl() {}
+
+// CHECK-LABEL: not
+void not() {}
+
+// CHECK-LABEL: not_eq
+void not_eq() {}
+
+// CHECK-LABEL: or
+void or() {}
+
+// CHECK-LABEL: or_eq
+void or_eq() {}
+
+// CHECK-LABEL: xor
+void xor() {}
+
+// CHECK-LABEL: xor_eq
+void xor_eq() {}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/53/builds/12221 Here is the relevant piece of the build log for the reference
|
…126758) - Disable `CXXOperatorNames` for HLSL - Add tests to confirm we can use the alt names as functions
…126758) - Disable `CXXOperatorNames` for HLSL - Add tests to confirm we can use the alt names as functions
…126758) - Disable `CXXOperatorNames` for HLSL - Add tests to confirm we can use the alt names as functions
CXXOperatorNames
for HLSL