Skip to content

Conversation

@dneto0
Copy link
Contributor

@dneto0 dneto0 commented May 19, 2020

For example, disallow

a < < b

from being recognized as a left-shift.

For example, disallow

    a < < b

from being recognized as a left-shift.
@dneto0 dneto0 added the wgsl WebGPU Shading Language Issues label May 19, 2020
@dneto0 dneto0 added this to the MVP milestone May 19, 2020
@dneto0 dneto0 requested a review from dj2 May 19, 2020 20:20
@dneto0
Copy link
Contributor Author

dneto0 commented May 19, 2020

In a larger sense, the spec doesn't talk about whitespace and tokenization.

@johnkslang
Copy link

Tokens should never span white space, and tokens should never be for white space.

That is, parsing (grammar time) should not take space into account, only a token stream that has already been normalized WRT space and not include space-based tokens. There has been some conflation of lexing and parsing pointed out in other issues.

E.g., this came up with the flawed idea of having - part of a number instead of as an operator, and then not being able to use - as an operator in front of a number.

wgsl/index.bs Outdated
<tr><td>`NOT_EQUAL`<td>!==
<tr><td>`GREATER_THAN`<td>>
<tr><td>`GREATER_THAN_EQUAL`<td>>=
<tr><td>`GREATER_GREATER`<td>>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to make these names consistent. I.e. why ARROW and not MINUS_GREATER? Similarly, there is ATTR_LEFT and not BRACKET_LEFT_LEFT.

If GREATER_GREATER is in fact the desired level at which the naming is done, then this would be something to follow-up. I wanted to raise it here in case GREATER_GREATER should be replaced by something like ARITHMETIC_SHIFT_RIGHT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, a more semantic name would be a better choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in a1f03bd

@dneto0
Copy link
Contributor Author

dneto0 commented May 20, 2020

I'm actually no longer sure this is the right fix. Compare with "old" C++ templates where you had to insert a space between nested templates ends to avoid confusing the tokenizer.

E.g. std::vector<std::unique_ptr<uint32_t> > That last space was needed to avoid confusing old compilers. The old compiler would otherwise see ">>" and think it was a shift operator.

@dneto0
Copy link
Contributor Author

dneto0 commented May 20, 2020

oh wait:

$ clang++ x.cc -o x.o
x.cc:5:30: error: a space is required between consecutive right angle brackets (use '> >')
  std::vector<std::vector<int>> a;
                             ^~
                             > >
1 error generated.
$ clang++ --version
Apple LLVM version 10.0.0 (clang-1000.11.45.5)
Target: x86_64-apple-darwin19.4.0
Thread model: posix

@kainino0x
Copy link
Contributor

oh wait:

It was fixed in C++11. You shouldn't get that error with -std=c++11.

@johnkslang
Copy link

But, C++11 is using a more complex parsing scheme than simpler languages, and we shouldn't burden WGSL with that complexity. Not to mention there is a pre-processor in the middle of the tokenization for C++.

That's also why I originally raised the white-space issues in my review: that's all covered by the pre-processing rules, but WGSL doesn't have a pre-processor, leaving a vacuum to fill regarding white space.

@kainino0x
Copy link
Contributor

100% agreed we don't want those parsing complexities.

@dj2 dj2 merged commit 7861a34 into gpuweb:master May 21, 2020
<tr><td>`GREATER_THAN_EQUAL`<td>>=
<tr><td>`LOGICAL_SHIFT_RIGHT`<td>>>
<tr><td>`ARITH_SHIFT_RIGHT`<td>>>>
<tr><td>`LESS_THAN`<td><
Copy link

@johnkslang johnkslang May 21, 2020

Choose a reason for hiding this comment

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

Might want to consider call this ANGLE_BRACKET_LEFT and ANGLE_BRACKET_RIGHT to separate terminology between lexing and parsing.

That would be more consistent with BRACE_LEFT instead of BEGIN_SCOPE, BANG instead of NOT, BRACKET_LEFT instead of BEING_ARRAY_INDEX, etc.

@dj2
Copy link
Member

dj2 commented May 28, 2020

Coming back to this to implement in Tint and it actually makes other bits of parsing more complicated. With this change, anywhere there is a GREATER_THAN (say vec4<vec2<f32>> as the example above we have to handle the fact that we get back a LOGICAL_SHIFT_RIGHT or ARITH_SHIFT_RIGHT instead of a GREATER_THAN and then, if we consume a portion of the token, shift the GREATER_THAN or LOGICAL_SHIFT_RIGHT back into the token stream.

This is doable, but is something other implementations will have to guard against as well.

@johnkslang
Copy link

I'm sure (and from some discussion above) you don't want this complexity in a simple portable language. I think you'll find more corner cases, and different implementations might find different ones.

@dj2
Copy link
Member

dj2 commented Jun 1, 2020

There are 3 options I see. You either accept this complexity, allow '> > >' to mean '>>>' or change how the parser/lexer works so the parser asks for specific tokens from the lexer instead of what's next.

Having the grammar as written in this CL is probably the right thing as '> > >' meaning '>>>' would be weird. But, implementers need to make sure they deal with the side effects.

JusSn pushed a commit to JusSn/gpuweb that referenced this pull request Jun 8, 2020
Make tokens for << >> >>>

For example, disallow

    a < < b

from being recognized as a left-shift.
JusSn pushed a commit to JusSn/gpuweb that referenced this pull request Jun 8, 2020
Make tokens for << >> >>>

For example, disallow

    a < < b

from being recognized as a left-shift.
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
This implements the F32Interval class and fundamental accuracy
functions described in my prototype,
gpuweb/cts#1402, for reworking how floating
point tests are written.

This also adds in testing that did not appear in my prototype.

There are a couple of functions that are not currently used that I
have included in this patch that have suppressions for unused-vars on
them. I included them in this patch since they are logically grouped
with this code. These surpressions will be removed in future patches.

Review discovered a bug in oneULP's implementation, which is fixed in
this PR. 

Issue gpuweb#792
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
Also includes a fix for f64, which was pushing inputs through a
Float32Array. This has been corrected to use a Float64Array.

Issue gpuweb#792
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
Allows passing in an F32Interval instead of an number[], so that cases
can bypass error calculation, instead of complicating error
calculations further.

Issue gpuweb#792
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
Also remove usage of this infra by integer tests that shouldn't have
been using it to begin with.

Fixes gpuweb#792
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wgsl WebGPU Shading Language Issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants