Vulkan: Remove row->col major shader transformation
This was done based on the incorrect assumption that Vulkan GLSL doesn't
allow layout qualifiers on interface block fields. This was due to
glslang compile failures in some shaders that included mixed row- and
column-major fields in interface blocks.
However, the failures were only in the case the interface block is
inactive, in which case glslang wrapper previously replaced the
layout/qualifier of the interface block with |struct|, which left the
shader with an unused struct definition with fields that have layout
qualifiers; an invalid shader.
The change introduced in
https://chromium-review.googlesource.com/c/angle/angle/+/1951523
removes inactive shader interface declarations. The above scenario thus
never occurs, rendering the row- to column-major transformation
unnecessary.
Bug: angleproject:3443
Change-Id: Ice34a0fc6e047b79a4d44f04b730ec59bdfafe33
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/1961098
Reviewed-by: Jonah Ryan-Davis <jonahr@google.com>
Reviewed-by: Cody Northrop <cnorthrop@google.com>
Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>
diff --git a/src/compiler/translator/OutputGLSLBase.h b/src/compiler/translator/OutputGLSLBase.h
index e5273a4..d6b2b1e 100644
--- a/src/compiler/translator/OutputGLSLBase.h
+++ b/src/compiler/translator/OutputGLSLBase.h
@@ -43,7 +43,7 @@
std::string getCommonLayoutQualifiers(TIntermTyped *variable);
std::string getMemoryQualifiers(const TType &type);
virtual void writeLayoutQualifier(TIntermTyped *variable);
- virtual void writeFieldLayoutQualifier(const TField *field);
+ void writeFieldLayoutQualifier(const TField *field);
void writeInvariantQualifier(const TType &type);
void writePreciseQualifier(const TType &type);
virtual void writeVariableType(const TType &type,
diff --git a/src/compiler/translator/OutputVulkanGLSL.cpp b/src/compiler/translator/OutputVulkanGLSL.cpp
index 21b0a08..6c990d2 100644
--- a/src/compiler/translator/OutputVulkanGLSL.cpp
+++ b/src/compiler/translator/OutputVulkanGLSL.cpp
@@ -53,7 +53,8 @@
return;
}
- TInfoSinkBase &out = objSink();
+ TInfoSinkBase &out = objSink();
+ const TLayoutQualifier &layoutQualifier = type.getLayoutQualifier();
// This isn't super clean, but it gets the job done.
// See corresponding code in glslang_wrapper_utils.cpp.
@@ -87,12 +88,12 @@
{
blockStorage = getBlockStorageString(storage);
}
+ }
- // We expect all interface blocks to have been transformed to column major, so we don't
- // specify the packing. Any remaining interface block qualified with row_major shouldn't
- // have any matrices inside.
- ASSERT(type.getLayoutQualifier().matrixPacking != EmpRowMajor ||
- !interfaceBlock->containsMatrices());
+ // Specify matrix packing if necessary.
+ if (layoutQualifier.matrixPacking != EmpUnspecified)
+ {
+ matrixPacking = getMatrixPackingString(layoutQualifier.matrixPacking);
}
if (needsCustomLayout)
@@ -131,14 +132,6 @@
}
}
-void TOutputVulkanGLSL::writeFieldLayoutQualifier(const TField *field)
-{
- // We expect all interface blocks to have been transformed to column major, as Vulkan GLSL
- // doesn't allow layout qualifiers on interface block fields. Any remaining interface block
- // qualified with row_major shouldn't have any matrices inside, so the qualifier can be
- // dropped.
-}
-
void TOutputVulkanGLSL::writeQualifier(TQualifier qualifier,
const TType &type,
const TSymbol *symbol)
diff --git a/src/compiler/translator/OutputVulkanGLSL.h b/src/compiler/translator/OutputVulkanGLSL.h
index c0d8f04..0a4eb4d 100644
--- a/src/compiler/translator/OutputVulkanGLSL.h
+++ b/src/compiler/translator/OutputVulkanGLSL.h
@@ -31,7 +31,6 @@
protected:
void writeLayoutQualifier(TIntermTyped *variable) override;
- void writeFieldLayoutQualifier(const TField *field) override;
void writeQualifier(TQualifier qualifier, const TType &type, const TSymbol *symbol) override;
void writeVariableType(const TType &type,
const TSymbol *symbol,
diff --git a/src/compiler/translator/TranslatorVulkan.cpp b/src/compiler/translator/TranslatorVulkan.cpp
index 1719200..f19a47e 100644
--- a/src/compiler/translator/TranslatorVulkan.cpp
+++ b/src/compiler/translator/TranslatorVulkan.cpp
@@ -18,12 +18,10 @@
#include "compiler/translator/OutputVulkanGLSL.h"
#include "compiler/translator/StaticType.h"
#include "compiler/translator/tree_ops/NameEmbeddedUniformStructs.h"
-#include "compiler/translator/tree_ops/NameNamelessUniformBuffers.h"
#include "compiler/translator/tree_ops/RemoveInactiveInterfaceVariables.h"
#include "compiler/translator/tree_ops/RewriteAtomicCounters.h"
#include "compiler/translator/tree_ops/RewriteCubeMapSamplersAs2DArray.h"
#include "compiler/translator/tree_ops/RewriteDfdy.h"
-#include "compiler/translator/tree_ops/RewriteRowMajorMatrices.h"
#include "compiler/translator/tree_ops/RewriteStructSamplers.h"
#include "compiler/translator/tree_util/BuiltIn.h"
#include "compiler/translator/tree_util/FindFunction.h"
@@ -779,25 +777,6 @@
}
}
- if (getShaderVersion() >= 300)
- {
- // Make sure every uniform buffer variable has a name. The following transformation relies
- // on this.
- if (!NameNamelessUniformBuffers(this, root, &getSymbolTable()))
- {
- return false;
- }
-
- // In GLES3+, matrices can be declared row- or column-major. Transform all to column-major
- // as interface block field layout qualifiers are not allowed. This should be done after
- // samplers are taken out of structs (as structs could be rewritten), but before uniforms
- // are collected in a uniform buffer as they are handled especially.
- if (!RewriteRowMajorMatrices(this, root, &getSymbolTable()))
- {
- return false;
- }
- }
-
if (defaultUniformCount > 0)
{
sink << "\n@@ LAYOUT-defaultUniforms(std140) @@ uniform defaultUniforms\n{\n";
diff --git a/src/tests/gl_tests/GLSLTest.cpp b/src/tests/gl_tests/GLSLTest.cpp
index 2e4dbbb..7316f1d 100644
--- a/src/tests/gl_tests/GLSLTest.cpp
+++ b/src/tests/gl_tests/GLSLTest.cpp
@@ -7254,10 +7254,6 @@
// Test that side effects respect the order of logical expression operands.
TEST_P(GLSLTest_ES3, MixedRowAndColumnMajorMatrices_ReadSideEffectOrder)
{
- // IntermTraverser::insertStatementsInParentBlock that's used to move side effects does not
- // respect the order of evaluation of logical expressions. http://anglebug.com/3829.
- ANGLE_SKIP_TEST_IF(IsVulkan());
-
// http://anglebug.com/3837
ANGLE_SKIP_TEST_IF(IsLinux() && IsIntel() && IsOpenGL());
@@ -7319,10 +7315,6 @@
// Test that side effects respect short-circuit.
TEST_P(GLSLTest_ES3, MixedRowAndColumnMajorMatrices_ReadSideEffectShortCircuit)
{
- // IntermTraverser::insertStatementsInParentBlock that's used to move side effects does not
- // respect short-circuiting in evaluation of logical expressions. http://anglebug.com/3829.
- ANGLE_SKIP_TEST_IF(IsVulkan());
-
// Fails on Android: http://anglebug.com/3839
ANGLE_SKIP_TEST_IF(IsAndroid() && IsOpenGL());
@@ -7595,6 +7587,9 @@
// Fails on D3D due to mistranslation: http://anglebug.com/3841
ANGLE_SKIP_TEST_IF(IsD3D11());
+ // Fails compiling shader on Android/Vulkan. http://anglebug.com/4290
+ ANGLE_SKIP_TEST_IF(IsAndroid() && IsVulkan());
+
constexpr char kFS[] = R"(#version 310 es
precision highp float;
out vec4 outColor;
