Skip to content
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

wgsl: Change 'abstract' -> 'abstract_float' #3216

Closed
wants to merge 1 commit into from

Conversation

zoddicus
Copy link
Contributor

@zoddicus zoddicus commented Dec 7, 2023

This avoids confusion with 'abstract_int'

Fixes #3184


Requirements for PR author:

  • All missing test coverage is tracked with "TODO" or .unimplemented().
  • New helpers are /** documented */ and new helper files are found in helper_index.txt.
  • Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)

Requirements for reviewer sign-off:

  • Tests are properly located in the test tree.
  • Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
  • Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
  • Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

This avoids confusion with 'abstract_int'

Fixes gpuweb#3184
Copy link
Contributor

@ben-clayton ben-clayton left a comment

Choose a reason for hiding this comment

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

It's not clear to me why the cache files have changed. Renaming fields in the code shouldn't affect the serialized cases.

FP.abstract.toInterval(kValue.f64.negative.subnormal.min),
FP.abstract.toInterval(kValue.f64.positive.infinity),
FP.abstract.toInterval(kValue.f64.negative.infinity),
FP.abstract_float.toInterval(0),
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels unnecessary. FP is floating-point. It would make little sense for FP.abstract to refer to anything but abstract-float.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is because we do things like FP[kind] to get a specific instance for a fp type , where kind is originating as string from a permutation of the test parameters. So there currently exists a potential conflict between abstract float and abstract int tests in the same .spec.ts for a builtin. (I don't think there exists any occurances at the moment, since the abstract_int tests haven't been written).

There are a number of options to resolve this:

  1. Leave as is, and just add abstract_int when needed in other parts of the tests, and people will just need to know that 'abstract' means abstract float.
  2. Push the change all the way down through the CTS, and change the naming from abstract to abstract_float everywhere (which is what I am doing in this test), so it is unambigious if we are dealing with abstract_float vs abstract_int.
    (Would using af instead of abstract_float be more appealing? Since we are already include that it is a float in f16 and f32)
  3. Have some sort of switching logic for getting a element out of the FP set that converts from abstract_float to abstract
  4. Split up tests so that we never have abstract float and abstract int being tested in the same file. This might work, though will still require some cognitive load to remember which abstract the context is refering to. And there is probably a bunch of internal places to the CTS where the distinction needs to be clear that will make it crufty.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is because we do things like FP[kind] to get a specific instance for a fp type , where kind is originating as string from a permutation of the test parameters.

But you're going to have to do something to switch between FP[kind] and Int[kind], no?

Copy link
Contributor

@jiangzhaoming jiangzhaoming Jan 3, 2024

Choose a reason for hiding this comment

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

Currently when we test built-ins (e.g. abs) that can take either abstract float or abstract integer as input type, we do have separated tests for integer and float types (but within a single .spec.ts file), naming the abstract float test abstract_float while accessing FP.abstract during case generating. In these case I think naming integer tests abstract_int will just work fine?

If we are going to have a more general dictionary that includes all FP and int types (like below?), maybe we can just make distinction in that dictionary, and those tests use this dictionary can use distinct parameter strings abstract_float and abstract_int. For those tests that just focus on FP or int but not both, I think a single abstract will not cause confusion?

A possible general dictionary(?) :

export const NumericTypes = {
    'f32': FP.f32,
    'f16': FP.f16,
    'abstract_float': FP.abstract,
    'i32': Int.i32,
    'u32': Int.u32,
    'abstract_int': Int.abstract,
};

@zoddicus zoddicus closed this Jan 3, 2024
@zoddicus
Copy link
Contributor Author

zoddicus commented Jan 3, 2024

I agree with both of your points/suggestions. This PR is not the way forward on this. This can be addressed when it comes up using something like the suggested dictionary or simple switching logic, without the overall cost of change everything.

@zoddicus zoddicus deleted the renameAbstractToAbstractFloat branch January 3, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wgsl: Change usage of abstract to abstract_float
3 participants