Skip to content

[HLSL] Implement the 'and' HLSL function #127098

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

Merged
merged 7 commits into from
Feb 19, 2025
Merged

Conversation

Icohedron
Copy link
Contributor

Addresses #125604

  • Implements and as an HLSL builtin function
  • The and HLSL builtin function gets lowered to the the LLVM and instruction

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:X86 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:headers Headers provided by Clang, e.g. for intrinsics clang:codegen IR generation bugs: mangling, exceptions, etc. HLSL HLSL Language Support labels Feb 13, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 13, 2025

@llvm/pr-subscribers-hlsl
@llvm/pr-subscribers-backend-x86
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Deric Cheung (Icohedron)

Changes

Addresses #125604

  • Implements and as an HLSL builtin function
  • The and HLSL builtin function gets lowered to the the LLVM and instruction

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

6 Files Affected:

  • (modified) clang/include/clang/Basic/Builtins.td (+6)
  • (modified) clang/lib/CodeGen/CGBuiltin.cpp (+5)
  • (modified) clang/lib/Headers/hlsl/hlsl_intrinsics.h (+16)
  • (modified) clang/lib/Sema/SemaHLSL.cpp (+11)
  • (added) clang/test/CodeGenHLSL/builtins/and.hlsl (+45)
  • (added) clang/test/SemaHLSL/BuiltIns/and-errors.hlsl (+27)
diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td
index 29939242596ba..de758d88f8f92 100644
--- a/clang/include/clang/Basic/Builtins.td
+++ b/clang/include/clang/Basic/Builtins.td
@@ -4765,6 +4765,12 @@ def HLSLAll : LangBuiltin<"HLSL_LANG"> {
   let Prototype = "bool(...)";
 }
 
+def HLSLAnd : LangBuiltin<"HLSL_LANG"> {
+  let Spellings = ["__builtin_hlsl_and"];
+  let Attributes = [NoThrow, Const];
+  let Prototype = "void(...)";
+}
+
 def HLSLAny : LangBuiltin<"HLSL_LANG"> {
   let Spellings = ["__builtin_hlsl_any"];
   let Attributes = [NoThrow, Const];
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 361e4c4bf2e2e..82527cb5e1f7a 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -19463,6 +19463,11 @@ Value *CodeGenFunction::EmitHLSLBuiltinExpr(unsigned BuiltinID,
         CGM.getHLSLRuntime().getAllIntrinsic(), ArrayRef<Value *>{Op0}, nullptr,
         "hlsl.all");
   }
+  case Builtin::BI__builtin_hlsl_and: {
+    Value *Op0 = EmitScalarExpr(E->getArg(0));
+    Value *Op1 = EmitScalarExpr(E->getArg(1));
+    return Builder.CreateAnd(Op0, Op1, "hlsl.and");
+  }
   case Builtin::BI__builtin_hlsl_any: {
     Value *Op0 = EmitScalarExpr(E->getArg(0));
     return Builder.CreateIntrinsic(
diff --git a/clang/lib/Headers/hlsl/hlsl_intrinsics.h b/clang/lib/Headers/hlsl/hlsl_intrinsics.h
index d1f5fdff8b600..7016b45d1c641 100644
--- a/clang/lib/Headers/hlsl/hlsl_intrinsics.h
+++ b/clang/lib/Headers/hlsl/hlsl_intrinsics.h
@@ -249,6 +249,22 @@ bool all(double3);
 _HLSL_BUILTIN_ALIAS(__builtin_hlsl_all)
 bool all(double4);
 
+//===----------------------------------------------------------------------===//
+// and builtins
+//===----------------------------------------------------------------------===//
+
+// \fn bool and(bool x, bool y)
+// \brief Logically ands two boolean vectors elementwise and produces a bool vector output.
+
+_HLSL_BUILTIN_ALIAS(__builtin_hlsl_and)
+bool and(bool x, bool y);
+_HLSL_BUILTIN_ALIAS(__builtin_hlsl_and)
+bool2 and(bool2 x, bool2 y);
+_HLSL_BUILTIN_ALIAS(__builtin_hlsl_and)
+bool3 and(bool3 x, bool3 y);
+_HLSL_BUILTIN_ALIAS(__builtin_hlsl_and)
+bool4 and(bool4 x, bool4 y);
+
 //===----------------------------------------------------------------------===//
 // any builtins
 //===----------------------------------------------------------------------===//
diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index 4abd870ad6aaa..7297fb3a9e4d0 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -2245,6 +2245,17 @@ bool SemaHLSL::CheckBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall) {
 
     break;
   }
+  case Builtin::BI__builtin_hlsl_and: {
+    if (SemaRef.checkArgCount(TheCall, 2))
+      return true;
+    if (CheckVectorElementCallArgs(&SemaRef, TheCall))
+      return true;
+    ExprResult A = TheCall->getArg(0);
+    QualType ArgTyA = A.get()->getType();
+    // return type is the same as the input type
+    TheCall->setType(ArgTyA);
+    break;
+  }
   case Builtin::BI__builtin_hlsl_all:
   case Builtin::BI__builtin_hlsl_any: {
     if (SemaRef.checkArgCount(TheCall, 1))
diff --git a/clang/test/CodeGenHLSL/builtins/and.hlsl b/clang/test/CodeGenHLSL/builtins/and.hlsl
new file mode 100644
index 0000000000000..60295f192f5cc
--- /dev/null
+++ b/clang/test/CodeGenHLSL/builtins/and.hlsl
@@ -0,0 +1,45 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 5
+// RUN: %clang_cc1 -finclude-default-header -triple \
+// RUN:   dxil-pc-shadermodel6.3-library %s \
+// RUN:   -emit-llvm -O1 -o - | FileCheck %s
+
+// CHECK-LABEL: define noundef i1 @_Z15test_and_scalarbb(
+// CHECK-SAME: i1 noundef [[X:%.*]], i1 noundef [[Y:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[HLSL_AND:%.*]] = and i1 [[X]], [[Y]]
+// CHECK-NEXT:    ret i1 [[HLSL_AND]]
+//
+bool test_and_scalar(bool x, bool y) {
+  return and(x, y);
+}
+
+// CHECK-LABEL: define noundef <2 x i1> @_Z14test_and_bool2Dv2_bS_(
+// CHECK-SAME: <2 x i1> noundef [[X:%.*]], <2 x i1> noundef [[Y:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[HLSL_AND:%.*]] = and <2 x i1> [[X]], [[Y]]
+// CHECK-NEXT:    ret <2 x i1> [[HLSL_AND]]
+//
+bool2 test_and_bool2(bool2 x, bool2 y) {
+  return and(x, y);
+}
+
+// CHECK-LABEL: define noundef <3 x i1> @_Z14test_and_bool3Dv3_bS_(
+// CHECK-SAME: <3 x i1> noundef [[X:%.*]], <3 x i1> noundef [[Y:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[HLSL_AND:%.*]] = and <3 x i1> [[X]], [[Y]]
+// CHECK-NEXT:    ret <3 x i1> [[HLSL_AND]]
+//
+bool3 test_and_bool3(bool3 x, bool3 y) {
+  return and(x, y);
+}
+
+// CHECK-LABEL: define noundef <4 x i1> @_Z14test_and_bool4Dv4_bS_(
+// CHECK-SAME: <4 x i1> noundef [[X:%.*]], <4 x i1> noundef [[Y:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[HLSL_AND:%.*]] = and <4 x i1> [[X]], [[Y]]
+// CHECK-NEXT:    ret <4 x i1> [[HLSL_AND]]
+//
+bool4 test_and_bool4(bool4 x, bool4 y) {
+  return and(x, y);
+}
+
diff --git a/clang/test/SemaHLSL/BuiltIns/and-errors.hlsl b/clang/test/SemaHLSL/BuiltIns/and-errors.hlsl
new file mode 100644
index 0000000000000..0fbf172d46ccd
--- /dev/null
+++ b/clang/test/SemaHLSL/BuiltIns/and-errors.hlsl
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -finclude-default-header -triple \
+// RUN:   dxil-pc-shadermodel6.3-library %s \
+// RUN:   -emit-llvm -O1 -verify
+
+bool test_too_few_arg(bool a) {
+  return __builtin_hlsl_and(a);
+  // expected-error@-1 {{too few arguments to function call, expected 2, have 1}}
+}
+
+bool test_too_many_arg(bool a) {
+  return __builtin_hlsl_and(a, a, a);
+  // expected-error@-1 {{too many arguments to function call, expected 2, have 3}}
+}
+
+bool2 test_mismatched_args(bool2 a, bool3 b) {
+  return __builtin_hlsl_and(a, b);
+  // expected-error@-1 {{all arguments to '__builtin_hlsl_and' must have the same type}}
+}
+
+struct S {
+  bool a;
+};
+
+bool test_invalid_type_conversion(S s) {
+  return __builtin_hlsl_and(s, s);
+  // expected-error@-1{{no viable conversion from returned value of type 'S' to function return type 'bool'}}
+}

if (const auto *VecTy = ArgTy->getAs<VectorType>()) {
ArgTy = VecTy->getElementType();
}
if (!getASTContext().hasSameUnqualifiedType(ArgTy,
Copy link
Member

Choose a reason for hiding this comment

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

For lines 2258 through 2264 look into CheckAllArgTypesAreCorrect you will need to add a bool version similar to CheckFloatingOrIntRepresentation or CheckUnsignedIntRepresentation or CheckFloatOrHalfRepresentations.

Copy link
Contributor Author

@Icohedron Icohedron Feb 15, 2025

Choose a reason for hiding this comment

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

I did try that. I had defined the following function:

static bool CheckBoolRepresentation(Sema *S, CallExpr *TheCall) {
  auto checkAllBoolTypes = [](clang::QualType PassedType) -> bool {
    return !PassedType->isBooleanType();
  };
  return CheckAllArgTypesAreCorrect(S, TheCall, S->Context.BoolTy,
                                    checkAllBoolTypes);
}

and when I tried to use it, I got a strange errors when running the codegen test

******************** TEST 'Clang :: CodeGenHLSL/builtins/and.hlsl' FAILED ***************
*****
Exit Code: 2

Command Output (stderr):
--
RUN: at line 2: /workspace/feature-and/build/bin/clang -cc1 -internal-isystem /workspace/feature-and/build/lib/clang/21/include -nostdsysteminc -finclude-default-header -triple    dxil-pc-shadermodel6.3-library /workspace/feature-and/clang/test/CodeGenHLSL/builtins/and.hlsl    -emit-llvm -O1 -o - | /workspace/feature-and/build/bin/FileCheck /workspace/feature-and/clang/test/CodeGenHLSL/builtins/and.hlsl
+ /workspace/feature-and/build/bin/clang -cc1 -internal-isystem /workspace/feature-and/build/lib/clang/21/include -nostdsysteminc -finclude-default-header -triple dxil-pc-shadermodel6.3-library /workspace/feature-and/clang/test/CodeGenHLSL/builtins/and.hlsl -emit-llvm -O1 -o -
+ /workspace/feature-and/build/bin/FileCheck /workspace/feature-and/clang/test/CodeGenHLSL/builtins/and.hlsl
/workspace/feature-and/clang/test/CodeGenHLSL/builtins/and.hlsl:23:14: error: passing 'bool2' (aka 'vector<bool, 2>') to parameter of incompatible type '__attribute__((__vector_size__(2 * sizeof(bool)))) bool' (vector of 2 'bool' values)
   23 |   return and(x, y);
      |              ^
/workspace/feature-and/clang/test/CodeGenHLSL/builtins/and.hlsl:33:14: error: passing 'bool3' (aka 'vector<bool, 3>') to parameter of incompatible type '__attribute__((__vector_size__(3 * sizeof(bool)))) bool' (vector of 3 'bool' values)
   33 |   return and(x, y);
      |              ^
/workspace/feature-and/clang/test/CodeGenHLSL/builtins/and.hlsl:43:14: error: passing 'bool4' (aka 'vector<bool, 4>') to parameter of incompatible type '__attribute__((__vector_size__(4 * sizeof(bool)))) bool' (vector of 4 'bool' values)
   43 |   return and(x, y);
      |              ^
/workspace/feature-and/clang/test/CodeGenHLSL/builtins/and.hlsl:55:14: error: passing 'vector<bool, 4>' (vector of 4 'bool' values) to parameter of incompatible type '__attribute__((__vector_size__(4 * sizeof(bool)))) bool' (vector of 4 'bool' values)
   55 |   return and(x, y);
      |              ^
/workspace/feature-and/clang/test/CodeGenHLSL/builtins/and.hlsl:67:14: error: passing 'vector<bool, 4>' (vector of 4 'bool' values) to parameter of incompatible type '__attribute__((__vector_size__(4 * sizeof(bool)))) bool' (vector of 4 'bool' values)
   67 |   return and(x, y);
      |              ^
5 errors generated.
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /workspace/feature-and/build/bin/FileCheck /workspace/feature-and/clang/test/CodeGenHLSL/builtins/and.hlsl

--

Copy link
Member

Choose a reason for hiding this comment

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

You aren't doing your lambda right. Check the base type. See CheckFloatOrHalfRepresentations for an example or look below:

auto checkAllBoolTypes = [](clang::QualType PassedType) -> bool {
    clang::QualType BaseType =
        PassedType->isVectorType()
            ? PassedType->getAs<clang::VectorType>()->getElementType()
            : PassedType;
    return !BaseType->isBooleanType();
  };

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the way the code is written now is a lot easier to read.

Copy link
Member

@farzonl farzonl Feb 16, 2025

Choose a reason for hiding this comment

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

We can talk about it Monday. But as is and builtin will be inconsistent with the errors we are using for all other hls builtins that do this same check.

Copy link
Member

@farzonl farzonl Feb 17, 2025

Choose a reason for hiding this comment

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

This actually doesn’t express my concern because you don’t know the expected type. There is no way to know it from the context available.

We know the expected types because we have defined them in sema per builtin. Thats how CheckAllArgsHaveFloatRepresentation, CheckFloatOrHalfRepresentations, CheckUnsignedIntRepresentation came to be in the first place. The Expected types are specified in these sema rules. and a spot check to hlsl_intrinsics.h should show they are the correct expected types. What im proposing is that we pass the list of all the expected types instead of just picking one.

Instead in the hand-rolled builtin diagnostics we're arbitrarily choosing an "expected" type and not explaining to the user that there were more than one valid options.

CheckFloatOrHalfRepresentations currently used by cross is a wrapper for CheckArgTypeIsCorrect. The problem you raised about alerting on float and nothalf is caused because we are only passing one expected type to CheckArgTypeIsCorrect. Changing this to a list allows us to do a diangostic per expected type.

Further switching to using CheckScalarOrVector has the same problem switching to it in the cross case would force you to do an error diagnostic on a "arbitrarily expected" type.

Copy link
Member

Choose a reason for hiding this comment

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

Anyways to move forward on this PR since and is just a bool scalar or vector arg and since we agree on using the err_typecheck_expect_scalar_or_vector diagnostic if @Icohedron switches to using CheckScalarOrVector instead of doing the SemaRef.Diag directly I'll sign off on this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We know the expected types because we have defined them in sema per builtin. Thats how CheckAllArgsHaveFloatRepresentation, CheckFloatOrHalfRepresentations, CheckUnsignedIntRepresentation came to be in the first place. The Expected types are specified in these sema rules. and a spot check to hlsl_intrinsics.h should show they are the correct expected types.

I think this is flawed logic. The source is ambiguous, we don't "know" the expected type we're guessing it.

What im proposing is that we pass the list of all the expected types instead of just picking one.

This sounds like a recipe for throwing out a lot of errors and making it really hard for the user to process.

CheckFloatOrHalfRepresentations currently used by cross is a wrapper for CheckArgTypeIsCorrect. The problem you raised about alerting on float and nothalf is caused because we are only passing one expected type to CheckArgTypeIsCorrect. Changing this to a list allows us to do a diangostic per expected type.

I don't think this solves the problem, I think it just makes us spew more errors at the user forcing them to decode a stream of messages.

Further switching to using CheckScalarOrVector has the same problem switching to it in the cross case would force you to do an error diagnostic on a "arbitrarily expected" type.

I'm not suggesting we switch cross to exactly CheckScalarOrVector as it is. I'm stating that our current pattern is not providing good clear diagnostics to our users, and we need to reconsider it. For cross we should be able to emit a diagnostic that the arguments must be "16-bit or 32-bit floating point or vectors of such types", which would be much clearer and more concise than a spew of "expected float" and "expected half" diagnostics.

For the issue in this PR: CheckScalarOrVector does exactly what we want. It allows printing an error message that the arguments must be "bool or vector of such type", which exactly describes the expected usage.

Copy link
Member

Choose a reason for hiding this comment

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

This sounds like a recipe for throwing out a lot of errors and making it really hard for the user to process.

How is that any different than what we do right now with overload rules? For example sin doesn't have a double overload so we tell the user every candidate function.

https://godbolt.org/z/7qE5bxhK6

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seriously?

In that example we give one error, and two notes. The error tells you the call was ambiguous, and the note identifies the highest ranked ambiguous overloads.

In a console, the one error is highlighted red. In an IDE the one error is collected and highlighted. The one error gets raised through LSP.

Your proposal would issue multiple errors, each of which is individually not completely accurate.

Copy link
Member

@farzonl farzonl left a comment

Choose a reason for hiding this comment

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

Codgen and testing is fine. SemaHLSL needs work to conform to how we have been doing things.

@llvm-beanz
Copy link
Collaborator

For this issue I think there is a clear path forward to use the utilities that the __builtin_hlsl_select builtin uses to validate its arguments. I've filed llvm/wg-hlsl#211 to revisit all the other builtins that are using the pattern which produces misleading diagnostics, we can handle the "everything else" problem separately there.

@bogner bogner linked an issue Feb 19, 2025 that may be closed by this pull request
5 tasks
Comment on lines 260 to 261
_HLSL_BUILTIN_ALIAS(__builtin_hlsl_and)
bool and (bool x, bool y);
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird formatting on these (Possible that clang-format gets confused because it thinks this is C++ and sees a keyword?)

Copy link
Member

Choose a reason for hiding this comment

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

Yep thats what is going on here. We should file an issue for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed #127851

@bogner bogner merged commit 1c762c2 into llvm:main Feb 19, 2025
6 of 8 checks passed
@bogner
Copy link
Contributor

bogner commented Feb 19, 2025

LGTM - I've gone ahead and merged this for you. Thanks!

@@ -0,0 +1,23 @@
// RUN: %clang_cc1 -finclude-default-header -triple \
// RUN: dxil-pc-shadermodel6.3-library %s \
// RUN: -emit-llvm -O1 -verify
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this testcase will write to the current directory. This may potentially be write protected and should be avoided.
Can we just skip "-emit-llvm"?
See e.g. a similar fix in b7730a2.

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, I will make a PR to fix this

llvm-beanz pushed a commit that referenced this pull request Feb 24, 2025
…riting to a potentially write-protected directory (#128047)

@mikaelholmen
[mentioned](#127098 (comment))
that the `-emit-llvm` argument isn't necessary for the `and-errors.hlsl`
test and may cause issues due to writing to the current (potentially
write-protected) directory.

This PR removes the `-emit-llvm` argument from clang in the RUN lines of
the test.
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Feb 24, 2025
… to avoid writing to a potentially write-protected directory (#128047)

@mikaelholmen
[mentioned](llvm/llvm-project#127098 (comment))
that the `-emit-llvm` argument isn't necessary for the `and-errors.hlsl`
test and may cause issues due to writing to the current (potentially
write-protected) directory.

This PR removes the `-emit-llvm` argument from clang in the RUN lines of
the test.
@damyanp damyanp moved this to Closed in HLSL Support Apr 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 clang:codegen IR generation bugs: mangling, exceptions, etc. clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:headers Headers provided by Clang, e.g. for intrinsics clang Clang issues not falling into any other category HLSL HLSL Language Support
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

Implement the and HLSL Function
6 participants