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

Use OpFUnordNotEqual for floating-point != #2260

Merged
merged 4 commits into from Jun 22, 2020

Conversation

gnl21
Copy link
Contributor

@gnl21 gnl21 commented Jun 5, 2020

The normal IEEE not equal operation tests whether operands are unordered
or not equal (so comparison with a NaN returns true). This corresponds
to the SPIR-V OpFUnordNotEqual, so change to using that.

Fixes #2259

@johnkslang
Copy link
Member

I think this proposes to make a != b the same as !(a == b). I think we should be quite careful about this. Is this backed up by the specification (or lack of detail enforcing NaN operation)? I even recall discussions that SPIR-V not even support the unordered compares, and one reason we did was for compiler transforms computing the ! of == and !=, but this seems to invalidate those arguments.

Do we need any specification changes for this?

@gnl21
Copy link
Contributor Author

gnl21 commented Jun 5, 2020

I think that this is justified by the current spec. It does propose to make a != b the same as !(a == b), but that's what's expected of the standard != operator in (all? most?) HLLs. It's a weird quirk of the naming convention that all IEEE comparisons convert to ordered comparison except !=, which naturally converts to the unordered version. The arguments about supporting the unordered comparisons (for <, >, ==, etc) apply in reverse to !=. There is no natural HLL expression of the ordered-not-equal, but we support it in SPIR-V to simplify compiler transforms.

For the purposes of ordered/unordered comparisons, any comparison involving a NaN is unordered. The definition of OpFUnordEqual says:

Floating-point comparison for being unordered or not equal.

which implies that, for OpFUnordNotEqual, x != NaN for all x. This matches C and D3D semantics. I'm not sure that GLSL says explicitly, but I think it is intended to match.

@gnl21
Copy link
Contributor Author

gnl21 commented Jun 17, 2020

Is there anything that I need to do to progress this?

@johnkslang
Copy link
Member

Do we know if all IHVs are ready for this? At least it's been posted here for a while.

We also need to bump the code generation number since we are changing a code emission pattern. That will help protect against surprises too.

See:

// For low-order part of the generator's magic number. Bump up
// when there is a change in the style (e.g., if SSA form changes,
// or a different instruction sequence to do something gets used).
int GetSpirvGeneratorVersion()
{
    // return 1; // start
    // return 2; // EOpAtomicCounterDecrement gets a post decrement, to map between GLSL -> SPIR-V
    // return 3; // change/correct barrier-instruction operands, to match memory model group decisions
    // return 4; // some deeper access chains: for dynamic vector component, and local Boolean component
    // return 5; // make OpArrayLength result type be an int with signedness of 0
    // return 6; // revert version 5 change, which makes a different (new) kind of incorrect code,
                 // versions 4 and 6 each generate OpArrayLength as it has long been done
    // return 7; // GLSL volatile keyword maps to both SPIR-V decorations Volatile and Coherent
    // return 8; // switch to new dead block eliminator; use OpUnreachable
    return 9; // don't include opaque function parameters in OpEntryPoint global's operand list
}

(We are also in the middle of possibly migrating to a new method via @ben-clayton's PRs, e.g., #2277.)

@johnkslang
Copy link
Member

BTW, it will be good to have the version bump in its own commit for ease of verification.

@gnl21
Copy link
Contributor Author

gnl21 commented Jun 22, 2020

Do we know if all IHVs are ready for this? At least it's been posted here for a while.

I don't know for certain, but this doesn't change the SPIR-V that drivers are being asked to accept, although it obviously changes the balance of how often these different opcodes come up. Both opcodes are currently tested in the CTS (OpFUnordNotEqual and OpFOrdNotEqual), so they should be working in implementations.

We also need to bump the code generation number

Sure, I'll do that now.

The normal IEEE not equal operation tests whether operands are unordered
or not equal (so comparison with a NaN returns true). This corresponds
to the SPIR-V OpFUnordNotEqual, so change to using that.
Change to 10 to reflect the change to generating unordered !=
operations.
Updating the SPIR-V generator version number changes the output of all
the SPIR-V tests.
@johnkslang johnkslang merged commit 8397044 into KhronosGroup:master Jun 22, 2020
@gnl21 gnl21 deleted the not-equal branch June 22, 2020 11:55
dstutt added a commit to dstutt/llpc that referenced this pull request Jul 22, 2020
Changes in glsl (see KhronosGroup/glslang#2259 and
KhronosGroup/glslang#2260) mean that fcmp will now use
une (unordered) rather than one (ordered) ne.

This is in line with most high level languages and means that NaN is handled in
a more expected way.

llpc-lit tests need updating for this change. The change allows for both one and
une during the period of transition.
trenouf pushed a commit to GPUOpen-Drivers/llpc that referenced this pull request Jul 22, 2020
Changes in glsl (see KhronosGroup/glslang#2259 and
KhronosGroup/glslang#2260) mean that fcmp will now use
une (unordered) rather than one (ordered) ne.

This is in line with most high level languages and means that NaN is handled in
a more expected way.

llpc-lit tests need updating for this change. The change allows for both one and
une during the period of transition.
alegal-arm pushed a commit to KhronosGroup/VK-GL-CTS that referenced this pull request Jun 30, 2021
…dNotEqual

The glslang compiler was changed in KhronosGroup/glslang#2260
to generate OpFUnordNotEqual rather than OpFOrdNotEqual for a!=b.

Without this change the
KHR-GL46.gl_spirv.spirv_glsl_to_spirv_builtin_functions_test
test generates an InternalError because it can't find the
expected mapping.

Affects:

KHR-GL46.gl_spirv.spirv_glsl_to_spirv_builtin_functions_test

Change-Id: Ieda8dc02c0be90a7985b15ca28dbefb1a45cc7c1
Components: OpenGL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SPIR-V output incorrectly translates float-not-equal to OpFOrdNotEqual
2 participants