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.

Sign up for GitHub

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

Jump to bottom

WIP: Implement swizzles containing 0 and 1 to represent numbers not indexes. #2208

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

WIP: Implement swizzles containing 0 and 1 to represent numbers not indexes. #2208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
64 changes: 57 additions & 7 deletions glslang/MachineIndependent/ParseContextBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,26 +481,46 @@ const TFunction* TParseContextBase::selectFunction(
// Look at a '.' field selector string and change it into numerical selectors
// for a vector or scalar.
//
// These are returned as indexes in selector.
// E.g. ".zy" will become selector = {2, 1}.
//
// Always return some form of swizzle, so the result is always usable.
//
// '0' and '1' in the field will mean to use the numeric values of 0 and 1
// rather than the result of an index into the vector.
// These are represented by:
// '0': MaxSwizzleSelectors
// '1': MaxSwizzleSelectors + 1
// E.g., ".z01" will become selector = {2, 4, 5} (if MasSwizzleSelectors == 4).
//
// A leading underscore (prefix) will get ignored.
//
void TParseContextBase::parseSwizzleSelector(const TSourceLoc& loc, const TString& compString, int vecSize,
TSwizzleSelectors<TVectorSelector>& selector)
TSwizzleSelectors<TVectorSelector>& selector, bool& numeric)
{
// a swizzle does not contain numerics unless there are actually numbers
// in it, independent of whether there is a prefix
numeric = false;

// If the field uses prefix syntax, normalize it.
const int firstChar = compString[0] == '_';

// Too long?
if (compString.size() > MaxSwizzleSelectors)
if (compString.size() - firstChar > MaxSwizzleSelectors)
error(loc, "vector swizzle too long", compString.c_str(), "");

// Use this to test that all swizzle characters are from the same swizzle-namespace-set
enum {
exyzw,
ergba,
estpq,
} fieldSet[MaxSwizzleSelectors];
enumeric,
} fieldSet[MaxSwizzleSelectors + 1];

// Decode the swizzle string.
int size = std::min(MaxSwizzleSelectors, (int)compString.size());
const int size = std::min(MaxSwizzleSelectors, (int)compString.size() - firstChar);
for (int i = 0; i < size; ++i) {
switch (compString[i]) {
switch (compString[i + firstChar]) {
case 'x':
selector.push_back(0);
fieldSet[i] = exyzw;
Expand Down Expand Up @@ -553,6 +573,17 @@ void TParseContextBase::parseSwizzleSelector(const TSourceLoc& loc, const TStrin
fieldSet[i] = estpq;
break;

case '0':
selector.push_back(MaxSwizzleSelectors);
fieldSet[i] = enumeric;
numeric = true;
break;
case '1':
selector.push_back(MaxSwizzleSelectors + 1);
fieldSet[i] = enumeric;
numeric = true;
break;

default:
error(loc, "unknown swizzle selection", compString.c_str(), "");
break;
Expand All @@ -561,13 +592,14 @@ void TParseContextBase::parseSwizzleSelector(const TSourceLoc& loc, const TStrin

// Additional error checking.
for (int i = 0; i < selector.size(); ++i) {
if (selector[i] >= vecSize) {
if (selector[i] < MaxSwizzleSelectors && selector[i] >= vecSize) {
error(loc, "vector swizzle selection out of range", compString.c_str(), "");
selector.resize(i);
break;
}

if (i > 0 && fieldSet[i] != fieldSet[i-1]) {
if (i > 0 && fieldSet[i] != enumeric && fieldSet[i-1] != enumeric &&
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems wrong. Would it catch the following bad case?

.x0u

Seems you need a longer lookback. Effectively I'd keep a state variable recording which letter-category has been forced so far. Then check for conflict.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see no problem here. Any struct could have a member x0u. There were no changes to tokenizing (that's entirely in the preprocessor). And the preprocessor still doesn't know anything about swizzling.

Rather, this is about parsing and semantic analysis. The 'u' will just fail to be a valid swizzle selector, and you'll get a semantic error, same as always.

'x0u' : unknown swizzle selection

Think of .x0u as a struct member that's allow on the vector type. It's just an identifier like any other.

Question

The reason I posted this as a draft is to see what direction to really go, and to show that one direction is non-trivial, despite appearances.

There are two independent decisions to make:

  1. (A) Emulate it (what this PR starts) or (B) add a new AST operator?

Emulating is this big complicated thing above, which is only partially implemented and will need lots of testing. Adding the new AST operator will mean that back ends must write new code to accept in order to support it.

  1. (C) Use a leading "_" or (D) change lexical analysis.

(D) Means adopting the new principle that the set of swizzle selectors cannot overlap the set of numeric suffixes, somewhat artificial and limiting to language growth and portability.

The fast trivial way to get it working is (B) and (C). This PR was to demonstrate the complexity of (A).

Do you have input on what direction to go?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I'm smart and dumb at the same time. The 'u' was wrong, should have been a swizzle letter from another swizzle letter-sequence.

This is the bad example:

#version 450

void main() {
  vec4 old;
  vec3 new = old.x0a;
}

This example compiles with this PR's branch. The problem is that the conflict between the x from the exyzw and the a from the ergba set is not detected because of the intervening 0.
So, that's the bug I was trying to point out.

Will think a bit more on your larger questions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have clear preference for (C) over (D).

I think a leading underscore is unobtrusive to the user, and easy to remember.
When I was thinking of this on my own, I thought of the underscore as ignorable anywhere in the swizzle. This is in analogy to using underscore as an ignorable digit separator as appeared in Go 1.13 https://golang.org/doc/go1.13
But maybe that's overkill. I'm perfectly happy with an ignorable leading underscore.

And to double-check: we only look for a swizzle when the object it follows is a vector, not a structure. That's what makes this avoid naming conflict with any user's type definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see, right, it can no longer rely on transitivity of being in the same partition percolating through. Haven't written any tests yet, not knowing whether to even do this.

Will fix once having input on direction.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I lean toward (B) over (A).
Questions:

  • What backends exist other than the SPIR-V code generator?
  • But also, I don't know the implications for folding in the front-end. I don't know what is required for GLSL support.

On the latter point, constant-folding of swizzles are required to be able to do things like size arrays:

#version 450

const ivec4 sizes = ivec4(10,20,30,40);
shared int w[sizes._1];      // with this PR's branch w is an array of size 1.

void main() {
  w[0] = 1;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I'll give (B) a shot sometime.

Right, GLSL mandates what's folded or not (and has to be, or not, for portability).

fieldSet[i] != fieldSet[i-1]) {
error(loc, "vector swizzle selectors not from the same set", compString.c_str(), "");
selector.resize(i);
break;
Expand All @@ -579,6 +611,24 @@ void TParseContextBase::parseSwizzleSelector(const TSourceLoc& loc, const TStrin
selector.push_back(0);
}

void TParseContextBase::replicateRValue(TIntermTyped* node, int num, TVector<TIntermTyped*>& replicates)
{
if (num == 0)
return;
if (num == 1) {
replicates.push_back(node);
return;
}
if (node->getAsSymbolNode()) {
replicates.push_back(node);
for (int i = 1; i < num; ++i)
replicates.push_back(intermediate.addSymbol(*node->getAsSymbolNode()));
}

// WIP: a complex expression needs to be evaluated exactly once, and then
// copies of the result put into the replicates.
}

#ifdef ENABLE_HLSL
//
// Make the passed-in variable information become a member of the
Expand Down
93 changes: 92 additions & 1 deletion glslang/MachineIndependent/ParseHelper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -886,7 +886,8 @@ TIntermTyped* TParseContext::handleDotSwizzle(const TSourceLoc& loc, TIntermType
}

TSwizzleSelectors<TVectorSelector> selectors;
parseSwizzleSelector(loc, field, base->getVectorSize(), selectors);
bool numeric = false;
parseSwizzleSelector(loc, field, base->getVectorSize(), selectors, numeric);

if (base->isVector() && selectors.size() != 1 && base->getType().contains16BitFloat())
requireFloat16Arithmetic(loc, ".", "can't swizzle types containing float16");
Expand All @@ -895,6 +896,9 @@ TIntermTyped* TParseContext::handleDotSwizzle(const TSourceLoc& loc, TIntermType
if (base->isVector() && selectors.size() != 1 && base->getType().contains8BitInt())
requireInt8Arithmetic(loc, ".", "can't swizzle types containing (u)int8");

if (numeric)
return handleNumericDotSwizzle(loc, base, selectors);

if (base->isScalar()) {
if (selectors.size() == 1)
return result;
Expand Down Expand Up @@ -927,6 +931,93 @@ TIntermTyped* TParseContext::handleDotSwizzle(const TSourceLoc& loc, TIntermType
return result;
}

// Handle a swizzle operation where at least one selector is numeric.
//
// Can return
// - a scalar constant (e.g. for ._1), but converted to the right type
// and constant folded
// - a vector constructor
// - a sequence containing
// 1. evaluation of 'base'
// 2. a scalar constant, converted, folded
// - a sequence containing
// 1. evaluation of 'base'
// 2. a vector constructor
//
// Note that none of the above include swizzle operations.
//
// Note: A vector constructor might require copies of the rvalue being swizzled,
// to avoid the tree accidentally becoming a DAG when there are multiple
// letter swizzles present needing multiple operations to get the
// components. This is quite unlike how swizzles are handled, or any
// other native GLSL operation.
//
TIntermTyped* TParseContext::handleNumericDotSwizzle(const TSourceLoc& loc, TIntermTyped* base,
const TSwizzleSelectors<TVectorSelector>& selectors)
{
const auto isLetter = [](int selector) { return selector < MaxSwizzleSelectors; };
const auto isNumber = [isLetter](int selector) { return !isLetter(selector); };
const auto getNumber = [](int selector) { return selector - MaxSwizzleSelectors; };

// The type of the result has the 'base' component type,
// but the component-count of 'selectors'.
TType type(base->getBasicType(), EvqTemporary, selectors.size());

// If only one selector, the result is a scalar.
// But, its type might be changing, so add a constructor.
// This will always result in an already folded scalar front-end constant.
if (selectors.size() == 1) {
assert(isNumber(selectors[0]));
return addConstructor(loc, intermediate.addConstantUnion(getNumber(selectors[0]), loc), type);

// WIP: this is incorrect if 'base' had side effects, it still needs to
// be evaluated as part of a sequence operation, unless the
// specification for this operation says those side effects are ignored.
}

// Otherwise, the result is like making a vector constructor,
// where we know we have more than one argument.

// Collect the arguments.
// This is complicated by the presence of more than one letter selector,
// because we need to reuse the r-value for each one, so it is a rare
// situation of needing to replicate the r-value.

// count the letter selectors (unless the base is a constant))
int letterCount = 0;
if (!base->getType().getQualifier().isFrontEndConstant()) {
for (int s = 0; s < selectors.size(); ++s)
letterCount += isLetter(selectors[s]) ? 1 : 0;
}
// get the replicates
TVector<TIntermTyped*> replicates;
replicateRValue(base, letterCount, replicates);

// process all the selectors to make the vector
TIntermAggregate* args = nullptr;
for (int s = 0; s < (int)selectors.size(); ++s) {
if (isNumber(selectors[s])) {
args = intermediate.growAggregate(args, intermediate.addConstantUnion(getNumber(selectors[s]), loc));
} else {
// traditional swizzle selector, which needs to consume the replicates
// (unless base is a constant)
TIntermTyped* arg;
if (base->getType().getQualifier().isFrontEndConstant()) {
arg = intermediate.foldDereference(base, selectors[s], loc);
} else {
TIntermTyped* rep = replicates.back();
replicates.pop_back();
TIntermTyped* index = intermediate.addConstantUnion(selectors[s], loc);
arg = intermediate.addIndex(EOpIndexDirect, rep, index, loc);
}
args = intermediate.growAggregate(args, arg);
}
}

// form the constructor
return addConstructor(loc, args, type)->getAsAggregate();
}

void TParseContext::blockMemberExtensionCheck(const TSourceLoc& loc, const TIntermTyped* base, int member, const TString& memberName)
{
// a block that needs extension checking is either 'base', or if arrayed,
Expand Down
5 changes: 4 additions & 1 deletion glslang/MachineIndependent/ParseHelper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ class TParseContextBase : public TParseVersions {
/* output */ bool& tie);

virtual void parseSwizzleSelector(const TSourceLoc&, const TString&, int size,
TSwizzleSelectors<TVectorSelector>&);
TSwizzleSelectors<TVectorSelector>&, bool& numeric);
virtual void replicateRValue(TIntermTyped* node, int n, TVector<TIntermTyped*>& replicates);

// Manage the global uniform block (default uniforms in GLSL, $Global in HLSL)
TVariable* globalUniformBlock; // the actual block, inserted into the symbol table
Expand Down Expand Up @@ -316,6 +317,8 @@ class TParseContext : public TParseContextBase {
TIntermTyped* handleUnaryMath(const TSourceLoc&, const char* str, TOperator op, TIntermTyped* childNode);
TIntermTyped* handleDotDereference(const TSourceLoc&, TIntermTyped* base, const TString& field);
TIntermTyped* handleDotSwizzle(const TSourceLoc&, TIntermTyped* base, const TString& field);
TIntermTyped* handleNumericDotSwizzle(const TSourceLoc&, TIntermTyped* base,
const TSwizzleSelectors<TVectorSelector>&);
void blockMemberExtensionCheck(const TSourceLoc&, const TIntermTyped* base, int member, const TString& memberName);
TFunction* handleFunctionDeclarator(const TSourceLoc&, TFunction& function, bool prototype);
TIntermAggregate* handleFunctionDefinition(const TSourceLoc&, TFunction&);
Expand Down
3 changes: 2 additions & 1 deletion hlsl/hlslParseHelper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -952,7 +952,8 @@ TIntermTyped* HlslParseContext::handleDotDereference(const TSourceLoc& loc, TInt
}
} else if (base->isVector() || base->isScalar()) {
TSwizzleSelectors<TVectorSelector> selectors;
parseSwizzleSelector(loc, field, base->getVectorSize(), selectors);
bool numeric = false;
parseSwizzleSelector(loc, field, base->getVectorSize(), selectors, numeric);

if (base->isScalar()) {
if (selectors.size() == 1)
Expand Down