KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
2.95k stars 819 forks source link

Build errors when NV_EXTENSIONS is not enabled #1712

Closed jamesdolan closed 5 years ago

jamesdolan commented 5 years ago
GlslangToSpv.cpp:3200:30: error: no member named 'E_SPV_NV_cooperative_matrix' in namespace 'spv'; did you mean 'glslang::E_GL_NV_cooperative_matrix'?
        builder.addExtension(spv::E_SPV_NV_cooperative_matrix);

Seems similar to https://github.com/KhronosGroup/glslang/issues/1347

johnkslang commented 5 years ago

@jeffbolznv, maybe this should be protected code?

    if (type.isCoopMat()) {
        builder.addCapability(spv::CapabilityCooperativeMatrixNV);
        builder.addExtension(spv::E_SPV_NV_cooperative_matrix);
        if (type.getBasicType() == glslang::EbtFloat16)
            builder.addCapability(spv::CapabilityFloat16);

        spv::Id scope = makeArraySizeId(*type.getTypeParameters(), 1);
        spv::Id rows = makeArraySizeId(*type.getTypeParameters(), 2);
        spv::Id cols = makeArraySizeId(*type.getTypeParameters(), 3);

        spvType = builder.makeCooperativeMatrixType(spvType, scope, rows, cols);
    }

Do you want to take this?

jeffbolznv commented 5 years ago

Can we finally ditch NV_EXTENSIONS/AMD_EXTENSIONS?

jeffbolznv commented 5 years ago

To be clear, I'm happy to fix this whichever way we agree to, but I strongly prefer to fix it by removing these ifdefs. They caused a bunch of problems when we added the Turing extension support, and are apparently still causing some binary compatibility issues in https://github.com/KhronosGroup/glslang/issues/1617.

johnkslang commented 5 years ago

Yes, I see your point. I tried building core glslang with and without all the switches (includes HLSL and the vendor switches), and it is 10% smaller.

I care somewhat about size, for cases where someone wants to embed a small compiler, but we do keep drifting further and further away from that.

Maybe good to hear from people who care. E.g., who is building with these off, and why is that important?

While waiting to hear, I recommend fixing this one.

jeffbolznv commented 5 years ago

I tried building core glslang with and without all the switches (includes HLSL and the vendor switches), and it is 10% smaller.

glslangValidator.exe shows only a 3% size difference with the vendor extension switches on vs off.

johnkslang commented 5 years ago

Is that including the google test, SPIR-V tools, etc.? These would also not be part of a small embedded usage.

jeffbolznv commented 5 years ago

I think it includes spirv-tools, but not google test.

jamesdolan commented 5 years ago

It's worth noting that I had them turned off not because of worry about code size, but simply that I didn't set them in the custom monolithic build that I have and updated to the latest glslang and saw this error.

I have for the time being defined them so things work, but generally its much easier for those of us that need to maintain different build systems to compile with default preprocessor environments.

baldurk commented 5 years ago

Chiming in on a related note, it's definitely still useful to have spirv-tools as an optional dependency as not all users will want to bring all that in. I don't mind changing the feature defines to always-on as long as spirv-tools remains optional - even if that means knowing the HLSL will not always compile legally.

johnkslang commented 5 years ago

Agreed.