made changes for include paths for complete installation #163

unexploredtest · 2021-02-24T09:35:04Z

When installed globally(sudo make install) glslang/StandAlone headers don't get installed(afaik) and only could get it to work after changing #include <StandAlone/ResourceLimits.h> to #include <glslang/Include/ResourceLimits.h>.

Also I had to change #include <SPIRV/GlslangToSpv.h> to #include <glslang/SPIRV/GlslangToSpv.h but I figured that it would break compatibility if one wanted to run locally in the same directory and without global installation.

axsaucedo · 2021-02-24T09:48:50Z

@aliPMPAINT interesting - ok I think it would be good to get further clarity on whether this is the expected, although from what you say it does make sense. It seems the tests are failing so it doesn't seem to extend to the cmake include. I actually opened an issue in the GLSLANG repo to understand what was the right branch to use - it may be worth perhaps opening an issue to ask them what is the best practice for this KhronosGroup/glslang#2539.

Would you be able to ask there? Otherwise I can open an issue as well.

unexploredtest · 2021-02-24T11:25:49Z

Oh I see what's my mistake, by bad. I thought that glslang/Include/ResourceLimits.h and StandAlone/ResourceLimits.h were the same thing, but StandAlone/ResourceLimits.h is nothing more than glslang/Include/ResourceLimits.h which extra code; here is the code for StandAlone/ResourceLimits.h:

#include "../glslang/Include/ResourceLimits.h"

namespace glslang {

// These are the default resources for TBuiltInResources, used for both
//  - parsing this string for the case where the user didn't supply one,
//  - dumping out a template for user construction of a config file.
extern const TBuiltInResource DefaultTBuiltInResource;

// Returns the DefaultTBuiltInResource as a human-readable string.
std::string GetDefaultTBuiltInResourceString();

// Decodes the resource limits from |config| to |resources|.
void DecodeResourceLimits(TBuiltInResource* resources, char* config);

}  // end namespace glslang

The DefaultTBuiltInResource constant(which exists just on StandAlone/ResourceLimits.h) is used on Shader.cpp:

    if (!shader.parse(&glslang::DefaultTBuiltInResource, 100, false, messages))
    {
        info_log = std::string(shader.getInfoLog()) + "\n" + std::string(shader.getInfoDebugLog());
        KP_LOG_ERROR("Kompute Shader Error: {}", info_log);
        throw std::runtime_error(info_log);
    }

But doesn't exist on the Kompute.hpp, and that's why it worked in my case, where Vulkan Kompute was already installed and I just changed Kompute.hpp include path's directory.

Would you be able to ask there? Otherwise I can open an issue as well.

Yeah sure, I'll open an issue.

axsaucedo · 2021-02-24T11:47:04Z

Oh I see what's my mistake, by bad. I thought that glslang/Include/ResourceLimits.h and StandAlone/ResourceLimits.h were the same thing, but StandAlone/ResourceLimits.h is nothing more than glslang/Include/ResourceLimits.h which extra code; here is the code for StandAlone/ResourceLimits.h:

OK does that mean that this PR is not needed? Not clear what is best practice / expectatition on their glslang lib.

Yeah sure, I'll open an issue.

Ok thanks, I think that would help provide more clarity.

unexploredtest · 2021-02-24T11:57:26Z

OK does that mean that this PR is not needed? Not clear what is best practice / expectatition on their glslang lib

This PR doesn't work out, but a solution is needed, as glslang/StandAlone/ResourceLimits.h won't get installed and is nowhere to find on the system.

Ok thanks, I think that would help provide more clarity.

KhronosGroup/glslang#2547

unexploredtest · 2021-02-25T06:12:56Z

KhronosGroup/glslang#2547 (comment)
They said:

FYI, StandAlone/ResourceLimits.h is not part of the external interface, although glslang/Include/ResourceLimits.h is.

Is there a way that let's us specify if it was compiling, then use 'StandAlone/ResourceLimits.h' and after compilation, set it to 'glslang/StandAlone/ResourceLimits.h'?

unexploredtest · 2021-02-25T06:31:04Z

Ok, it works if we include glslang/Include/ResourceLimits.h in Shader.hpp and StandAlone/ResourceLimits.h in Shader.cpp, but it's forbidden to include external headers in *.cpp files right?

axsaucedo · 2021-02-25T06:59:37Z

@aliPMPAINT thank you for having a look. Yes I think we need to understand what the best practice is - it seems like Standalone is an internal component so it should not be included. This is no optimal as it seems otherwise we'd have to define our own TBuiltInResource limits - I guess by default we could skip this by just creating a blank TBuiltInResource, but it would be important to verify if this would have any potential implications.

What I would suggest is if you could have a brief look at what the main purpose of this resource is, and if it's OK to include a barebones TBuiltInResource, then we can add this as a new parameter with default value so in can be overriden in the Shader function, such as:

std::vector<uint32_t>
Shader::compile_sources(const std::vector<std::string>& sources,
                                   const std::vector<std::string>& files,
                                   const std::string& entryPoint,
                                   std::vector<std::pair<std::string,std::string>> definitions
                                   glslang::TBuiltInResource resources) {

This way we can just have includes of external interfaces, which would be best practice. However if we do find that some of the limits are necessary, then we'll probably have to hand craft our own default one from the provided DefaultTBuiltInResource in the Standalone folder

unexploredtest · 2021-02-25T09:43:33Z

I'm on it.

unexploredtest · 2021-02-27T07:26:53Z

Ok, so I had done the required research and experiments, sorry for the tardiness.

I guess by default we could skip this by just creating a blank TBuiltInResource

No, we can't, it'll just conceive the max values as zeros and limits as false, as GLSL gave me compilation errors that indicated that they were so.

What I would suggest is if you could have a brief look at what the main purpose of this resource is

The main purpose is to set maxes to some values(like MaxComputeWorkGroupSizeX) and won't allow them to bypass that limit and will throw compilation errors as a result(like:

terminate called after throwing an instance of 'std::runtime_error'
  what():  ERROR: :4: 'local_size' : too large; see gl_MaxComputeWorkGroupSize 
ERROR: 1 compilation errors.  No code generated.

or set some limitations, such as whether while loops are allowed or not.

However if we do find that some of the limits are necessary, then we'll probably have to hand craft our own default one from the provided DefaultTBuiltInResource in the Standalone folder

Yes, that's the solution. We can change some of the values(like I don't think we'll use MaxLights, so we can just set it to zero) and set the required ones by our own, and also meanwhile we can override the default one. I'll send a commit in a minute and explain it further.

…sources method

axsaucedo · 2021-02-27T07:30:11Z

Ok this is great @aliPMPAINT , thank you very much for your efforts doing an exploration on this - I agree and think all your conclusions are quite reasonable. My only worry is that not using the exact defaults in the Standalone repo means that we'll have to make sure we keep in mind if there are any potential breaking changes, although I don't foresee this being a major issue (for example we are in 11.1 and they just released 11.2 but I don't think there are major changes on this front)

axsaucedo

Looks good - could you also add a simple test that also shows that you can add the limits (one where perhasp the limits are too small and shows an error, and anotherone that shows that it doesn't error)?

src/Shader.cpp

axsaucedo · 2021-02-27T07:32:23Z

src/include/kompute/Shader.hpp


+private:
+    // The default resource limit for the GLSL compiler, can be overwritten


is everythign else except max-lights the same as the standalone file? May be worth adding a comment also linking to the github file

Yeah I was about to write an explanation for it. It's actually not complete, as I don't know which ones are exactly required and which it's not required in our usage, like I'm sure we won't need MaxLights or 'MaxFragmentUniformComponents' so I set them to zero, but I honestly don't know about MaxCombinedShaderOutputResources. Do you know what exactly we need?

May be worth adding a comment also linking to the github file

Yeah sure

axsaucedo · 2021-02-27T07:34:34Z

It seems the tests are failing as well, so will require also seeing how this would work for development when there's just add_subdir (without this installed) - if this is not consistent I can reopen the issue in the Khronosgroup/glslang repo as this incosistency between development and install seems quite dodgy

unexploredtest · 2021-02-27T07:35:09Z

My only worry is that not using the exact defaults in the Standalone repo means that we'll have to make sure we keep in mind if there are any potential breaking changes

I actually haven't thought of that... Yeah we should check on that

unexploredtest · 2021-02-27T07:40:24Z

It seems the tests are failing as well, so will require also seeing how this would work for development when there's just add_subdir (without this installed)

Yeah my bad, I forgot to change them back(it was really annoying to set them back manually).

…urces, fixed inlude

unexploredtest

could you also add a simple test that also shows that you can add the limits (one where perhasp the limits are too small and shows an error, and anotherone that shows that it doesn't error)

Will do.

src/Shader.cpp

unexploredtest · 2021-02-27T08:00:07Z

So, the tests are failing because(I think) I haven't added the single include file, which was done on purpose because the last time it tried to rewrite the whole file. Will fix it soon.

axsaucedo · 2021-02-27T08:17:30Z

single_include/kompute/Kompute.hpp

+    // The default resource limit for the GLSL compiler, can be overwritten
+    // Has been adobted by:
+    // https://github.com/KhronosGroup/glslang/blob/master/StandAlone/ResourceLimits.cpp
+    static constexpr TBuiltInResource defaultResource = {


here you can just call it const as that is the same as constexpr

It yielded me an error, this:
constexpr' needed for in-class initialization of static data member
more on this here:
https://stackoverflow.com/questions/9141950/initializing-const-member-within-class-declaration-in-c

Ok i see, although not clear why it would error being static inside a class, may be worth having a deeper look, as the rest of the functions are within the Shader scope so would be best to have this variable also within

Yes, I'm trying to do so for hours. The closet thing I have come into is static constexpr TBuiltInResource const& defaultResource(got it from here), but instead of the undefined reference I got is multiple definition of '_ZGRN2kp6Shader15defaultResourceE_'; CMakeFiles/test_kompute.dir/TestAsyncOperations.cpp.o:(.rodata+0x0): first defined her. Any ideas?

Hmm ok I see, I think given this is taking too long let's just get it as global and we can move it (we an open an issue separate)

Yeah sounds good

unexploredtest · 2021-02-27T08:22:34Z

The tests kept getting undefined reference to kp::Shader::defaultResource, is it because it's static?

axsaucedo · 2021-02-27T12:41:41Z

@aliPMPAINT we can merge with the global defaultResource for now - would you still be able to add a test for this discussed in the comment earlier?

axsaucedo · 2021-02-27T14:48:39Z

@aliPMPAINT I will merge this PR but would be good to add the tests for resources via separate pr

unexploredtest · 2021-02-27T15:41:21Z

would you still be able to add a test for this discussed in the comment earlier?

Working on it 👍

unexploredtest · 2021-02-27T15:48:17Z

Also, about

is everythign else except max-lights the same as the standalone file?

I think it'll be good idea to open an issue about this and investigate which ones are necessary and which ones can be neglected and be set to zero.

made changes for include paths for complete installation

c132f90

included StandAlone/ResourceLimits.h in src/Shader.cpp?

eafcd04

Fixed include paths and added resources parameter to Shader::compile_…

f60e71d

…sources method

axsaucedo requested changes Feb 27, 2021

View reviewed changes

refrence parameter by const refrence, linked tothe original GLSL reso…

3cad95c

…urces, fixed inlude

unexploredtest commented Feb 27, 2021

View reviewed changes

src/Shader.cpp Outdated Show resolved Hide resolved

updated ingle_include/kompute/Kompute.hpp

bb7893b

axsaucedo reviewed Feb 27, 2021

View reviewed changes

took the defaultResource outside of the Shader class

ff63fe7

axsaucedo approved these changes Feb 27, 2021

View reviewed changes

axsaucedo merged commit 5db9abd into KomputeProject:master Feb 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

made changes for include paths for complete installation #163

made changes for include paths for complete installation #163

unexploredtest commented Feb 24, 2021

axsaucedo commented Feb 24, 2021

unexploredtest commented Feb 24, 2021 •

edited

Loading

axsaucedo commented Feb 24, 2021

unexploredtest commented Feb 24, 2021

unexploredtest commented Feb 25, 2021 •

edited

Loading

unexploredtest commented Feb 25, 2021 •

edited

Loading

axsaucedo commented Feb 25, 2021 •

edited

Loading

unexploredtest commented Feb 25, 2021

unexploredtest commented Feb 27, 2021 •

edited

Loading

axsaucedo commented Feb 27, 2021

axsaucedo left a comment

axsaucedo Feb 27, 2021

unexploredtest Feb 27, 2021 •

edited

Loading

axsaucedo commented Feb 27, 2021

unexploredtest commented Feb 27, 2021

unexploredtest commented Feb 27, 2021

unexploredtest left a comment •

edited

Loading

unexploredtest commented Feb 27, 2021 •

edited

Loading

axsaucedo Feb 27, 2021

unexploredtest Feb 27, 2021

axsaucedo Feb 27, 2021

unexploredtest Feb 27, 2021 •

edited

Loading

axsaucedo Feb 27, 2021

unexploredtest Feb 27, 2021

unexploredtest commented Feb 27, 2021 •

edited

Loading

axsaucedo commented Feb 27, 2021

axsaucedo commented Feb 27, 2021

unexploredtest commented Feb 27, 2021

unexploredtest commented Feb 27, 2021 •

edited

Loading


		private:
		// The default resource limit for the GLSL compiler, can be overwritten

made changes for include paths for complete installation #163

made changes for include paths for complete installation #163

Conversation

unexploredtest commented Feb 24, 2021

axsaucedo commented Feb 24, 2021

unexploredtest commented Feb 24, 2021 • edited Loading

axsaucedo commented Feb 24, 2021

unexploredtest commented Feb 24, 2021

unexploredtest commented Feb 25, 2021 • edited Loading

unexploredtest commented Feb 25, 2021 • edited Loading

axsaucedo commented Feb 25, 2021 • edited Loading

unexploredtest commented Feb 25, 2021

unexploredtest commented Feb 27, 2021 • edited Loading

axsaucedo commented Feb 27, 2021

axsaucedo left a comment

Choose a reason for hiding this comment

axsaucedo Feb 27, 2021

Choose a reason for hiding this comment

unexploredtest Feb 27, 2021 • edited Loading

Choose a reason for hiding this comment

axsaucedo commented Feb 27, 2021

unexploredtest commented Feb 27, 2021

unexploredtest commented Feb 27, 2021

unexploredtest left a comment • edited Loading

Choose a reason for hiding this comment

unexploredtest commented Feb 27, 2021 • edited Loading

axsaucedo Feb 27, 2021

Choose a reason for hiding this comment

unexploredtest Feb 27, 2021

Choose a reason for hiding this comment

axsaucedo Feb 27, 2021

Choose a reason for hiding this comment

unexploredtest Feb 27, 2021 • edited Loading

Choose a reason for hiding this comment

axsaucedo Feb 27, 2021

Choose a reason for hiding this comment

unexploredtest Feb 27, 2021

Choose a reason for hiding this comment

unexploredtest commented Feb 27, 2021 • edited Loading

axsaucedo commented Feb 27, 2021

axsaucedo commented Feb 27, 2021

unexploredtest commented Feb 27, 2021

unexploredtest commented Feb 27, 2021 • edited Loading

unexploredtest commented Feb 24, 2021 •

edited

Loading

unexploredtest commented Feb 25, 2021 •

edited

Loading

unexploredtest commented Feb 25, 2021 •

edited

Loading

axsaucedo commented Feb 25, 2021 •

edited

Loading

unexploredtest commented Feb 27, 2021 •

edited

Loading

unexploredtest Feb 27, 2021 •

edited

Loading

unexploredtest left a comment •

edited

Loading

unexploredtest commented Feb 27, 2021 •

edited

Loading

unexploredtest Feb 27, 2021 •

edited

Loading

unexploredtest commented Feb 27, 2021 •

edited

Loading

unexploredtest commented Feb 27, 2021 •

edited

Loading