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

Windows build fails with return type inference failure. #149

Open
NeedsMoar opened this issue May 30, 2024 · 3 comments
Open

Windows build fails with return type inference failure. #149

NeedsMoar opened this issue May 30, 2024 · 3 comments

Comments

@NeedsMoar
Copy link
Contributor

Describe the bug
For reasons I can't quite begin to fathom (I'm betting intellisense is right and it's incorrectly deducing the type as a luisa::compute::Bool when the operators should strictle speaking only return a bool, but I'm not sure why intellisense deduces that type and the compiler errors out) that might be a CL.exe bug to be reported to microsoft, in u64.h the following two lines only cause a build failure on lines 103 and 106:

    [[nodiscard]] auto operator>(Expr<uint> rhs) const noexcept { return rhs < *this; }
    [[nodiscard]] auto operator<=(Expr<uint> rhs) const noexcept { return !(rhs < *this); }

The error is that auto deduction can't be used because the function hasn't yet been declared, which is obviously screwy. Manually specifying the return type as the one intellisense inferred:

    [[nodiscard]] luisa::compute::Bool operator>(Expr<uint> rhs) const noexcept { return rhs < *this; }

Allows it to build, but just changing the return to bool doesn't; it then starts thinking the "rhs" parameter is in error, saying that no suitable conversion from bool to luisa::Compute::Var exists. These operators should all be returning bool so something weird is happening.

To Reproduce
Build in Visual Studio current release.

Expected behavior
The build should have succeeded

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):
Windows 10 Pro Workstation, Visual Studio 2022 Community 17.10.1
Visual C++ 2022 00482-90000-00000-AA303
Microsoft Visual C++ 2022

Additional context
I'd probably file a bug with microsoft (their compiler team is friendly so it shouldn't eat up much time, but it's your project so best you do it) and treat what I did as a temporary fix if it can't be done some smoother way (Possibly switch to the UFO operator <=> instead and bypass the whole mess since you're building against a high enough C++ standard to use it); otherwise I might be tired but I'm failing to see how anything incorrect is happening here on your end, code-wise.

@NeedsMoar
Copy link
Contributor Author

NeedsMoar commented May 30, 2024

I just realized u64.h lives under the src\utils\ subdirectory of the main renderer, not the compute sub-project, so it might be namespace leakage like the other issue I filed a few weeks ago after all. You might want to move this bug over there if possible.

@Mike-Leo-Smith
Copy link
Contributor

Hi @NeedsMoar

I'm not sure if this is a compiler bug or if it's because I have some ill-formed use of the C++ language (TBH, it's too hard to learn all the C++ syntax and rules). Anyways, explicitly specifying the return type as luisa::compute::Bool should be the most straightforward fix and I'll merge it into the LuisaRender repo. Many thanks for your investigation!

Allows it to build, but just changing the return to bool doesn't; it then starts thinking the "rhs" parameter is in error, saying that no suitable conversion from bool to luisa::Compute::Var exists. These operators should all be returning bool so something weird is happening.

The plain bool type is expected not to work because the operations on shader variables (i.e., the Var<T> or Expr<T> objects) cannot be converted back to the native C++ values of type bool.

Also, we'll eventually remove the u64.h util header. Earlier it was for emulated 64-bit integer computation with two 32-bit integers but now we have extended LuisaCompute's type system with direct support for 64-bit integers. So the emulation is no longer necessary.

@Mike-Leo-Smith
Copy link
Contributor

I have removed the outdated U64 implementation from LuisaRender.

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

No branches or pull requests

2 participants