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

Feature/initialize list side branch #6058

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
e0378c6
SP004: implement initialize list translation to ctor
kaizhangNV Jan 9, 2025
0f295da
simplify the logic for creating invoke for synthesized ctor
kaizhangNV Jan 9, 2025
9c40cba
Implement a fallback mechanism to the legacy init list
kaizhangNV Jan 9, 2025
ee7bdf8
Address some comment feedback
kaizhangNV Jan 9, 2025
907a50f
Implement filling the ctor body
kaizhangNV Jan 9, 2025
56047da
Apply the new rule for inheritance
kaizhangNV Jan 9, 2025
8f89551
Keep implementing filling the ctor body
kaizhangNV Jan 10, 2025
7684104
Fix test errors
kaizhangNV Jan 10, 2025
4035b7f
Keep fixing test errors
kaizhangNV Jan 10, 2025
acc3830
Fix more test errors
kaizhangNV Jan 10, 2025
6c013a3
Fix the diagnostic issue and no_diff issue
kaizhangNV Jan 10, 2025
e4dbe41
Fix test failure for hlsl-syntax.slang
kaizhangNV Jan 10, 2025
87523c8
fix smoke-gfx
kaizhangNV Jan 10, 2025
799bc43
Fix bug
kaizhangNV Jan 10, 2025
b3405b5
Fix test errors
kaizhangNV Jan 10, 2025
73e7950
fix bug: default value of parameter
kaizhangNV Jan 13, 2025
4a91a67
Fix reflection test
kaizhangNV Jan 13, 2025
6146fea
fix gfx-smoke test
kaizhangNV Jan 13, 2025
bce3bf8
Bypass the issue #4874 for now
kaizhangNV Jan 14, 2025
c0bc16e
Remove the default constructor when we have a valid member init const…
kaizhangNV Jan 15, 2025
950b071
DNI: Test new Falcor test
kaizhangNV Jan 15, 2025
593270a
Fix issue in wrongly using base contructor
kaizhangNV Jan 15, 2025
46351bd
DNI: Test new Falcor-Compile-perf test
kaizhangNV Jan 16, 2025
fdb52c8
remove WAR for global variable
kaizhangNV Jan 16, 2025
989b104
Fix tests due to removing the default constructor
kaizhangNV Jan 16, 2025
cffa13c
Disable -zero-initialize option
kaizhangNV Jan 16, 2025
4ee436e
fix gfx-smoke test due to remove default constructor
kaizhangNV Jan 16, 2025
c8f667f
include enum type to the rule of C-Style struct
kaizhangNV Jan 16, 2025
65a6474
fix ABI breaking
kaizhangNV Jan 17, 2025
2f41cd4
remove hlsl-syntax.slang.expected files
kaizhangNV Jan 17, 2025
99acef4
remove unnecessary lines
kaizhangNV Jan 17, 2025
adec731
Add cuda-host decoration for the synthesized constructor
kaizhangNV Jan 17, 2025
db3063a
Address low hanging fruit comments and format
kaizhangNV Jan 17, 2025
de53b13
Attempt to fix the overload lookup issue
kaizhangNV Jan 17, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions .github/workflows/falcor-compiler-perf-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,11 @@ jobs:

# A flag to set the download target as latest release
# The default value is 'false'
latest: true

tag: "v0.0.2"
# The name of the file to download.
# Use this field only to specify filenames other than tarball or zipball, if any.
# Supports wildcard pattern (eg: '*', '*.deb', '*.zip' etc..)
fileName: "falcor_perf_test-*-win-64.zip"
fileName: "falcor_perf_test-v0.0.2-win-64-beta.zip"

# Download the attached zipball (*.zip)
zipBall: true
Expand Down
18 changes: 9 additions & 9 deletions .github/workflows/falcor-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,12 @@ jobs:
run: |
mkdir FalcorBin
cd FalcorBin
Copy-Item -Path 'C:\Falcor\build\windows-vs2022\bin' -Destination '.\build\windows-vs2022\bin' -Recurse -Exclude ("*.pdb")
Copy-Item -Path 'C:\Falcor\tests' -Destination '.\' -Recurse
Copy-Item -Path 'C:\Falcor\tools' -Destination '.\' -Recurse
Copy-Item -Path 'C:\Falcor\media' -Destination '.\' -Recurse
Copy-Item -Path 'C:\Falcor\media_internal' -Destination '.\' -Recurse
Copy-Item -Path 'C:\Falcor\scripts' -Destination '.\' -Recurse
Copy-Item -Path 'C:\Falcor-new\build\windows-vs2022\bin' -Destination '.\build\windows-vs2022\bin' -Recurse -Exclude ("*.pdb")
Copy-Item -Path 'C:\Falcor-new\tests' -Destination '.\' -Recurse
Copy-Item -Path 'C:\Falcor-new\tools' -Destination '.\' -Recurse
Copy-Item -Path 'C:\Falcor-new\media' -Destination '.\' -Recurse
Copy-Item -Path 'C:\Falcor-new\media_internal' -Destination '.\' -Recurse
Copy-Item -Path 'C:\Falcor-new\scripts' -Destination '.\' -Recurse
cd ..\
- name: Build Slang
run: |
Expand All @@ -78,18 +78,18 @@ jobs:
cmake --workflow --preset "${{matrix.config}}"
- name: Copy Slang to Falcor
run: |
cp --verbose --recursive --target-directory ./FalcorBin/build/windows-vs2022/bin/Release build/Release/bin/*
cp --verbose --recursive --target-directory ./FalcorBin/build/windows-vs2022/bin/Debug build/Release/bin/*
- name: falcor-unit-test
shell: pwsh
run: |
$ErrorActionPreference = "SilentlyContinue"
cd .\FalcorBin\tests
python ./testing/run_unit_tests.py --config windows-vs2022-Release -t "-slow"
python ./testing/run_unit_tests.py --config windows-vs2022-Debug -t "-slow"
cd ../../
- name: falcor-image-test
shell: pwsh
run: |
$ErrorActionPreference = "SilentlyContinue"
cd .\FalcorBin\tests
python ./testing/run_image_tests.py --config windows-vs2022-Release --run-only
python ./testing/run_unit_tests.py --config windows-vs2022-Debug -t "-slow"
cd ../../
3 changes: 3 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ cmake_policy(SET CMP0091 NEW)
# tree relocatable
set(CMAKE_BUILD_RPATH_USE_ORIGIN TRUE)

# Export the compile datebase as a json file, this can be used by VIM language server
set(CMAKE_EXPORT_COMPILE_COMMANDS ON)

# Enable placing targets into a hierarchy for IDE generators
set_property(GLOBAL PROPERTY USE_FOLDERS ON)

Expand Down
38 changes: 36 additions & 2 deletions docs/proposals/004-initialization.md
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,39 @@ MyType x = MyType(y); // equivalent to `x = y`.

The compiler will attempt to resolve all type casts using type coercion rules, if that failed, will fall back to resolve it as a constructor call.

### Inheritance Initialization
For derived struct, slang will synthesized the constructor by bring the parameters from the base struct's constructor if the base struct also has a synthesized constructor. For example:
kaizhangNV marked this conversation as resolved.
Show resolved Hide resolved
```csharp
struct Base
{
int x;
// compiler synthesizes:
// __init(int x) { ... }
}
struct Derived : Base
{
int y;
// compiler synthesizes:
// __init(int x, int y) { ... }
}
```

However, if the base struct has explicit ctors, the compiler will not synthesize a constructor for the derived struct.
For example, given
```csharp
struct Base { int x; __init(int x) { this.x = x; } }
struct Derived : Base { int y;}
```
The compiler will not synthesize a constructor for `Derived`, and the following code will fail to compile:
```csharp

Derived d = {1}; // error, no matching ctor.
Derived d = {1, 2}; // error, no matching ctor.
Derived d = Derived(1); // error, no matching ctor.
Derived d = Derived(1, 2); // error, no matching ctor.
```


### Initialization List

Slang allows initialization of a variable by assigning it with an initialization list.
Expand Down Expand Up @@ -167,7 +200,8 @@ If the above code passes type check, then it will be used as the way to initiali

If the above code does not pass type check, and if there is only one constructor for`MyType` that is synthesized as described in the previous section (and therefore marked as `[Synthesized]`, Slang continues to check if `S` meets the standard of a "legacy C-style struct` type.
A type is a "legacy C-Style struct" if all of the following conditions are met:
- It is a user-defined struct type or a basic scalar, vector or matrix type, e.g. `int`, `float4x4`.
- It is a user-defined struct type or an enum, a basic scalar, vector or matrix type, e.g. `int`, `float4x4`.
- It does not inherit from any other types.
- It does not contain any explicit constructors defined by the user.
- All its members have the same visibility as the type itself.
- All its members are legacy C-Style structs or arrays of legacy C-style structs.
Expand Down Expand Up @@ -405,4 +439,4 @@ Alternatives Considered

One important decision point is whether or not Slang should allow variables to be left in uninitialized state after its declaration as it is allowed in C++. In contrast, C# forces everything to be default initialized at its declaration site, which come at the cost of incurring the burden to developers to come up with a way to define the default value for each type.
Our opinion is we want to allow things as uninitialized, and to have the compiler validation checks to inform
the developer something is wrong if they try to use a variable in uninitialized state. We believe it is desirable to tell the developer what's wrong instead of using a heavyweight mechanism to ensure everything is initialized at declaration sites, which can have non-trivial performance consequences for GPU programs, especially when the variable is declared in groupshared memory.
the developer something is wrong if they try to use a variable in uninitialized state. We believe it is desirable to tell the developer what's wrong instead of using a heavyweight mechanism to ensure everything is initialized at declaration sites, which can have non-trivial performance consequences for GPU programs, especially when the variable is declared in groupshared memory.
23 changes: 20 additions & 3 deletions source/slang/slang-ast-decl.h
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,12 @@ class AggTypeDecl : public AggTypeDeclBase
class StructDecl : public AggTypeDecl
{
SLANG_AST_CLASS(StructDecl);

SLANG_UNREFLECTED
// We will use these auxiliary to help in synthesizing the member initialize constructor.
Slang::HashSet<VarDeclBase*> m_membersVisibleInCtor;
Dictionary<int, ConstructorDecl*> m_synthesizedCtorMap;
bool m_hasExplicitCtorInExtension = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder what this is used for, and I am guessing it is used to prevent synthesis of a ctor if there is already one provided via an extension.

I think the decision on whether or not to synthesize a ctor should be solely based on the struct type itself, and not any extensions.

We also need to make sure in the presence of an extension, the ctor provided in the extension should take precedence over the implicitly synthesized method during overload resolution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, this seems like a corner case we haven't discussed before.
I thought extension will be part of the struct, so it can also determine whether we should define the ctor.

The thing is that if the syntheized ctor is exactly the same as the explicit ctor in extension, won't that be a compile error?

Copy link
Collaborator

@csyonghe csyonghe Jan 18, 2025

Choose a reason for hiding this comment

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

Yes, this what I was saying that during overload resolution, if we see a user defined ctor that is as good as the synthesized one, it should always be preferred over implicitly generated ones.

The one reason that ctor synthesis shouldn't depend on extensions is if a module defines public X and an internal extension on X that adds a ctor, should we synthesize ctor for X or not? When this decision is affected by extensions, we will say we don't. But then from the user of the module's perspective this is weird because they don't get to see the extension and yet X for some reason doesn't have the expected ctor.

If we say the decision is not dependent on extensions, then we will always synthesize the ctor. But inside the module that sees both the extension and X itself, the user defined ctor will be preferred over the synthesized one if it is applicable, so we will have no issues. From the outside of the module, they still can use the synthesized ctor without surprises.

};

class ClassDecl : public AggTypeDecl
Expand Down Expand Up @@ -372,9 +378,20 @@ class ConstructorDecl : public FunctionDeclBase
{
SLANG_AST_CLASS(ConstructorDecl)

// Indicates whether the declaration was synthesized by
// slang and not actually provided by the user
bool isSynthesized = false;
enum class ConstructorTags : int
kaizhangNV marked this conversation as resolved.
Show resolved Hide resolved
{
None = 0x00,
// Indicates whether the declaration was synthesized by
// Slang and not explicitly provided by the user
Synthesized = 0x01,
// Member initialize constructor is a synthesized ctor,
// but it takes parameters.
MemberInitCtor = 0x02
};

int m_tags = (int)ConstructorTags::None;
void addTag(ConstructorTags tag) { m_tags |= (int)tag; }
bool containsTag(ConstructorTags tag) { return m_tags & (int)tag; }
};

// A subscript operation used to index instances of a type
Expand Down
Loading