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

Regression: Conforming literals now spam warnings everywhere on 16-bit integer math #6867

Closed
devshgraphicsprogramming opened this issue Aug 17, 2024 · 2 comments
Labels
bug Bug, regression, crash needs-triage Awaiting triage

Comments

@devshgraphicsprogramming
Copy link

devshgraphicsprogramming commented Aug 17, 2024

Description

Performing an arithmetic operation like + between a uint16_t and a integer literal produces warnings about narrowing conversions.

This only happens with the -HV 202x flag, and not -HV 2021, so I blame conforming literals change.

This is a huge problem because there are no portable literal suffices for short integers or half floats.

Steps to Reproduce

uint16_t val;
uint16_t another = val+1;

Here's a godbolt https://godbolt.org/z/v77cje6ab

Actual Behavior

I get

warning: conversion from larger type 'unsigned int' to smaller type 'uint16_t', possible loss of data [-Wconversion]
    uint16_t oneMore = input.color+1;

Environment

  • DXC version: trunk on Godbolt
  • Host Operating System: Godbolt's Linux
@devshgraphicsprogramming devshgraphicsprogramming added bug Bug, regression, crash needs-triage Awaiting triage labels Aug 17, 2024
@devshgraphicsprogramming devshgraphicsprogramming changed the title Regression: Conforming literals now spam warnings everywhere Regression: Conforming literals now spam warnings everywhere on 16-bit integer math Aug 17, 2024
devshgraphicsprogramming pushed a commit to Devsh-Graphics-Programming/Nabla that referenced this issue Aug 17, 2024
@simondeschenes
Copy link

simondeschenes commented Aug 20, 2024

In GLSL, there are suffixes for those types.
https://github.com/KhronosGroup/GLSL/blob/main/extensions/ext/GL_EXT_shader_explicit_arithmetic_types.txt

     -------------------------------------
     | suffix           | type           |
     -------------------------------------
     | no suffix        | int, int32_t   |
     | u or U           | uint, uint32_t |
     | s or S           | int16_t        |
     | both u/U and s/S | uint16_t       |
     | l or L           | int64_t        |
     | both u/U and l/L | uint64_t       |
     -------------------------------------

I suggest we do the same and use s and us suffixes.

For the moment, I think we can work around the problem with like this:

uint16_t val;
uint16_t another = val+(uint16_t)1);

But that is really ugly.

@llvm-beanz
Copy link
Collaborator

@devshgraphicsprogramming you've hit us with this from a bunch of different directions, so I'm going to provide a bit of a unified answer here.

As I noted here, C/C++ compilers have all sorts of conversions for 8 and 16-bit types up to 32-bit. They don't warn because they're so prolific and fundamentally part of the language. All arguments of arithmetic types to any expression go through usual arithmetic conversions, which promotes all smaller than "word" sized integer types following the integer promotion rules.

HLSL does not have usual arithmetic conversions because they would be oh so horribly awful for SIMD execution performance (imagine if every int16 needed to be promoted to an int32 then demoted back for every operation).

We chose to align with C++ specifically because of the prevalence of existing code like 0xffff << 16 or 0x1 << 24, both of which would produce very unintuitive results if the literals were 16-bit.

As @simondeschenes points out we could have aligned with GLSL, however deviating from C++ is generally something we try to avoid. I do realize we diverged by not having usual arithmetic conversions but I think that demonstrates diverging when there is a really strong justification.

For context, we felt there was a similarly strong justification for having un-suffixed floating literals be float instead of double. This is due to the very large volume of existing HLSL that uses un-suffixed literals and does not intend to pay the performance cost of double computations (we try to avoid sudden performance cliffs in language changes).

@devshgraphicsprogramming separately reported microsoft/hlsl-specs#304, which suggests two possible alternative solutions: C++23 explicit sized suffixes, C++11 user-defined literals.

I did consider adding the C++23 literal suffixes, but chose not to because C++23 isn't widely used and there some oddity in how those are parsed that I didn't want to support in DXC. If we need to do something in HLSL 202x (rather than 202y), that is the most likely approach, but I'm still unconvinced we need to do something for HLSL 202x.

User defined literals are another approach we could consider. I worry a bit about how that will interact with other HLSL language features in DXC.

Any of the three alternatives suggested here (GLSL suffixes, C++23 suffixes, C++11 user-defined literals) are language changes not compiler tooling changes. The compiler is behaving as expected given the current language specification, and for that reason I'm closing this as not to be fixed.

We can use microsoft/hlsl-specs#304 or other issues on https://github.com/microsoft/hlsl-specs/ to track changes to the language.

@llvm-beanz llvm-beanz closed this as not planned Won't fix, can't repro, duplicate, stale Aug 20, 2024
@github-project-automation github-project-automation bot moved this to Triaged in HLSL Triage Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug, regression, crash needs-triage Awaiting triage
Projects
Archived in project
Development

No branches or pull requests

3 participants