Skip to content

This is really a reference #5473

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

Merged
merged 3 commits into from
Aug 9, 2023
Merged

Conversation

llvm-beanz
Copy link
Collaborator

This fixes a few cases that were missed when I updated this objects to be references. Specifically we were still generating some implicit this cases as pointers, and we were not always correctly looking through the reference types.

Fixes #4709

This fixes a few cases that were missed when I updated `this` objects
to be references. Specifically we were still generating some implicit
`this` cases as pointers, and we were not always correctly looking
through the reference types.

Fixes microsoft#4709
../tools/clang/test/HLSLFileCheck/hlsl/classes/template_base_this.hlsl
../tools/clang/test/HLSLFileCheck/hlsl/classes/this_reference_2018.hlsl
@llvm-beanz llvm-beanz added the for-release Issues and PRs prioritized for the upcoming release label Jul 31, 2023
Copy link
Member

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

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

Looks good though I'd still like the comments in the tests.

I'd like to have a discussion about the nonsensical LValueToRvalue casts. I've heard you mention it before and I'd like to understand why they exist and what to do about them.

@@ -0,0 +1,48 @@
// RUN: %dxc -T lib_6_4 -HV 2021 %s -ast-dump | FileCheck %s -check-prefix=AST
// RUN: %dxc -T lib_6_4 -HV 2021 %s -fcgl | FileCheck %s
Copy link
Member

Choose a reason for hiding this comment

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

I would appreciate a comment at the top of both of these tests explaining briefly what's being tested

@llvm-beanz
Copy link
Collaborator Author

I'd like to have a discussion about the nonsensical LValueToRvalue casts. I've heard you mention it before and I'd like to understand why they exist and what to do about them.

Here's a simple example of the kinds of nonsense casts we get in the AST. Take this code:

struct Doggo {
  int4 Legs;
};

void Walk(inout Doggo D);

void fn() {
  Doggo D = {1.xxxx};
  Walk(D);
}

Godbolt Link.

When D is passed to Walk, it is an l-value. It gets modified by Walk so it must be an l-value. But the AST is this:

CallExpr <line:10:3, col:9> 'void'
|-ImplicitCastExpr <col:3> 'void (*)(__restrict Doggo)' <FunctionToPointerDecay>
| `-DeclRefExpr <col:3> 'void (__restrict Doggo)' lvalue Function 'Walk' 'void (__restrict Doggo)'
`-ImplicitCastExpr <col:8> 'Doggo' <LValueToRValue>
  `-DeclRefExpr <col:8> 'Doggo' lvalue Var 'D' 'Doggo'

According to the AST, D gets l-value to r-value casted and the r-value of D gets passed into the AST. In this case it is because the AST doesn't represent UDTs being passed inout.

There are other cases where we do this wrong too. Fixing the ASTs to be accurate is the intent of #5249.

@llvm-beanz llvm-beanz merged commit 871fba1 into microsoft:main Aug 9, 2023
llvm-beanz added a commit to llvm-beanz/DirectXShaderCompiler that referenced this pull request Aug 9, 2023
This fixes a few cases that were missed when I updated `this` objects to
be references. Specifically we were still generating some implicit
`this` cases as pointers, and we were not always correctly looking
through the reference types.

Fixes microsoft#4709
llvm-beanz added a commit that referenced this pull request Aug 10, 2023
This fixes a few cases that were missed when I updated `this` objects to
be references. Specifically we were still generating some implicit
`this` cases as pointers, and we were not always correctly looking
through the reference types.

Fixes #4709

Cherry-pick of 871fba1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for-release Issues and PRs prioritized for the upcoming release
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Accessing templated base class member var results in compiler crash
3 participants