KhronosGroup / GLSL

GLSL Shading Language Issue Tracker
324 stars 96 forks source link

Clarify "Boolean expression" for binary logical operators #198

Open rdb opened 1 year ago

rdb commented 1 year ago

Moved here from KhronosGroup/glslang#3118 after confirming it's a spec issue and not a validator issue.

The current wording of the definition for &&, || and ^^ operators is misleading as to whether or not they operate on boolean vectors.

The wording (in all GLSL and ESSL spec versions) is:

The logical binary operators and (&&), or (||), and exclusive or (^^) operate only on two Boolean expressions and result in a Boolean expression.

There is no definition of the term "Boolean expression", but the word "boolean type" is defined as follows, which seems to suggest that this refers to vectors as well:

Definition A boolean type is any boolean scalar or vector type (bool, bvec2, bvec3, bvec4)

In all other places where only bool scalars are returned, the spec uses the language "scalar Boolean" specifically, eg:

Relational and equality operators (<, <=, >, >=, ==, !=) are defined to operate on scalars and produce scalar Boolean results.

This, combined with the fact that SPIR-V, Metal and pre-2021 HLSL do have these operators for vectors, strongly suggests that these operators were intended to also accept boolean vectors.

However, this is not the case. The reference GLSL validator contains an explicit check added by @johnkslang himself, suggesting that it was not the intent to define this operator for vectors. Several drivers I tested also do not accept this. @greg-lunarg furthermore pointed out that GLSL is unique in that (unlike SPIR-V and pre-2021 HLSL) it performs short-circuiting, which would be less obvious to define for vectors.

Therefore, I would suggest that this ambiguity be resolved by adding explicit language, either by using the term "scalar Boolean expression", defining the term "Boolean expression", or adding a specific note indicating that boolean vectors are not permitted for these operators.

(Apropos, I discovered this because I found this functionality missing. I think it would be useful to add component-wise logicalAnd, logicalOr etc. in GLSL, without short-circuiting behaviour. This would mirror the distinction between ! and not, and provide a useful feature that is missing wrt other shading languages.)

johnkslang commented 1 year ago

Originally shipped GLSL had already made the decision that && and || were flow-control operators just as they are in C/C++. E.g.:

if (a > b || funcall()) ...

would only call funcall() if a > b was false, and a > b from the beginning was defined to result in a scalar Boolean.

GLSL was generalizing C/C++ operators to operate on vectors, but the above doesn't generalize well to vectors, without removing the short-circuiting that C++ expects. Should it call funcall() or not(?), or how can you call it only on some channels(?), etc. So, there's a fundamental choice in language design which direction to go. (Both directions work, but trying for a hybrid would be questionable.)

Related, GLSL was trying to be safer than C++ in forcing the operands to actually be Boolean, with it built-in that a > b had a scalar Boolean type, not a numerical type.

any() and all() are provided to reduce a Boolean vector into a scalar Boolean, and the family greaterThan() et. al. to do bitwise comparisons, so if v1 and v2 are vectors, one could write:

if (any(greaterThan(v1, v2)) || funcall())

Giving the coder complete explicit control what was bitwise/reducing/scalar/etc.

The spec. was subtly distinguishing between using "logical" operators on scalar Booleans (a > b) resulting in scalar control flow and "bitwise" operators on vectors resulting on vectors or reduction of vectors, but I certainly agree it only made that distinction partially/inconsistently.

rdb commented 1 year ago

Thank you for that clarification. It would be good to get some clarification in the spec.

On the matter of the missing functionality, would it be at all useful if I proposed a GL(ES) extension adding and() and or() functions?

johnkslang commented 1 year ago

Or, generalize &, ^, and | to also operate on vectors of Booleans. I don't know if it's worth adding the functionality or not. We also have mix(). I'm curious about any motivating example that isn't similarly expressible another way.

Edit: Remove "not for selecting from other Booleans", as that was already added.

rdb commented 1 year ago

I bumped into this when working on spirv-cross and making a polyfill for isinf for legacy GLSL, I expected to be able to do:

bvec4 isinf(vec4 v) {
  return notEqual(v, vec4(0.0)) && equal(v * 2.0, v);
}

Of course adding something to GLSL now won't help with that use case, so I don't have a major stake in this at this moment. Though I could probably come up with some use cases if pressed. I suggested the feature because it feels like an omission in the language since we do have boolean mix(), not() and xor (notEqual()) but no way to combine multiple boolean expressions logically, reducing the usefulness of boolean mix(). SPIR-V already defines non-short-circuiting vector and/or instructions that this would trivially map to.

Overloading the bitwise operators feels inconsistent, because there already is a not() function rather than overloading bitwise invert ~ for inverting boolean vectors. HLSL 2021 also went the route of defining them as functions instead of overloading the bitwise ops.

rdb commented 1 year ago

It just occurred to me that since GLSL 4.40 you can trivially implement and and or as a boolean mix, without having to split it out per-component:

#define and(a, b) mix(b, a, b)
#define or(a, b) mix(a, b, b)

It's not exactly obvious though.

johnkslang commented 1 year ago

Agreed with adding built-in functions, if anything does get added.

The macros are a good idea, but error prone if there are side effects, because they evaluate b more than once, and evaluate a and b in opposite orders. Use functions instead.

I was expecting to answer by using mix() on a specific example, but I think we're past that now, given your general solution using mix().

gnl21 commented 6 months ago

I've created an internal PR that clarifies that these operators only operate on scalars.