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

lvalue producing flat conversions #355

Open
llvm-beanz opened this issue Nov 20, 2024 · 0 comments
Open

lvalue producing flat conversions #355

llvm-beanz opened this issue Nov 20, 2024 · 0 comments
Labels
language-spec Issue with completed spec Theme:Consistency Issues related to HLSL not being consistent
Milestone

Comments

@llvm-beanz
Copy link
Collaborator

Which document does this relate to?
language

Describe the issue you see with the spec

In talking with @spall about some of the "flat conversion" code she's working on I realized that DXC does allow flat conversions to produce lvalues. We even advertised this as a workaround for HLSL 2021's strict UDT casting rules so that users could add explicit casts on function arguments to convert layout-identical structures.

Enter the problem.

There are actually two code paths in DXC's codegen that implement flat conversions. One in CGExprAgg.cpp which I believe does the right thing (element-wise conversion of each aggregate element), and one in CGExpr.cpp... which doesn't.

The problematic implementation comes from this PR: microsoft/DirectXShaderCompiler#137

Which seems to have been intended to support derived->base conversions, but the CGExpr case actually gets hit in a lot more cases than what the tests reflect, and it triggers in cases like this (which is basically what we told users to do for HLSL 2021):

struct St1 {
  int A;
  float B;
};

struct St2 {
  int A;
  int B;
};

void fn(inout St2 S) {
  S.B += S.A;
}

export float call(int I, float F) {
  St1 S = {I, F};
  fn((St2)S);
  S.B += S.A + 0.5f;
  return S.A + S.B;
}

The problem here is that in the function call S isn't converted to St2 via a flat conversion that performs element-wise conversion, instead it results in a bitcast... Which is very much not correct.

A few immediate thoughts on options:

  1. Fix this all and make it work. - This probably isn't too hard, but the language is gross, especially around function output parameters.
  2. Only support the cases that DXC gets right. - The cases that generate correct code in DXC are pretty simple to support, and the rest is maybe nonsense. We could take this opportunity to simplify the language a bit by removing behavior that doesn't even work correctly.

Additional context
Godbolt

@llvm-beanz llvm-beanz modified the milestones: HLSL in Clang, HLSL Spec Nov 20, 2024
@llvm-beanz llvm-beanz added language-spec Issue with completed spec Theme:Consistency Issues related to HLSL not being consistent and removed needs-triage labels Nov 20, 2024
@llvm-beanz llvm-beanz moved this to Triaged in HLSL Triage Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
language-spec Issue with completed spec Theme:Consistency Issues related to HLSL not being consistent
Projects
Status: Triaged
Development

No branches or pull requests

1 participant