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

scalar == vector is not supported #442

Closed
dankbaker opened this issue Aug 8, 2016 · 8 comments
Closed

scalar == vector is not supported #442

dankbaker opened this issue Aug 8, 2016 · 8 comments

Comments

@dankbaker
Copy link
Contributor

Circa line 1826, in hlslGrammer.cpp.

Consider the lines:
Buffer TestUAVBuffer : register(t19);
if(TestUAVBuffer.Load(uBufferIndex.x).x == uBufferValue)
return float4(1,0,1,1);

It looks to me like the parser things this is a variable, which causes a null node and an expression failure. I think there are 2 ways to fix it. I can take a crack at it, but wanted to post it here for advice

  1. Understand that the variable is a sampler object, thus this must be a function
  2. Look ahead a bit further to see if we have a a.b(...) pattern. This would be my preference, as it's more generally looking for objects with methods on them.
@johnkslang
Copy link
Member

@steve-lunarg any input on this one?

@dankbaker
Copy link
Contributor Author

Looking at it some more, it does look like there is a look ahead later in the code if it's var, but the binary expression later fails because the left hand side ends up being a null node. I'll keep poking around, still learning the code ;)

@ghost
Copy link

ghost commented Aug 8, 2016

I'm OOTO today and have to step out in about 2 minutes, but I quickly tried the code below, and it parsed successfully and produced SPIR-V. Perhaps I've missed the salient feature of the example? It looked like plausible AST, but I only glanced quickly. I'll check it out in more detail this evening; I might be missing the obvious in a rush.


Buffer TestUAVBuffer : register(t19);

float4 main()
{
    int4 uBufferIndex = 2;
    float uBufferValue = 0;

    if (TestUAVBuffer.Load(uBufferIndex.x).x == uBufferValue)
        return float4(1,0,1,1);

    return float4(0,0,0,0);
}

@dankbaker
Copy link
Contributor Author

dankbaker commented Aug 8, 2016

Ok, I dug into it more. The problem is circa line 133 in intermiediate.cpp in the lfunction addBinaryMath line if(!node->Promote()) return 0; Digging into it some more...

Ok, I think the problem might be a more general issue, promote is failing on the comparison because of the comparison on line 1703 of intermediate.cpp, where one variable is makred with vector1, and the other isn't, even though the dimensions match - because in this case the uBufferValue is declared as a uint1.

In HLSL, a 1 dimensional vector and is the same as a scalar. Actually, internally everything in HLSL is a vector. I think this issue will manifest itself many times doing the conversion of HLSL input.

I can prob fix but I'm not sure what you'd like to do, should it accept comparisons between vec1s and scalars? Seems like that's not allowed in GLSL though which this file is shared between.

Another option is to just always set vector1 to true on HLSL front end. I'm not sure what implications that has though.

-D

@johnkslang
Copy link
Member

I've been adding HLSL-specific conversions as needed, as per the issue in flight.

@steve-lunarg found cases where for function overloading the distinction mattered between vector of size one and scalar. So, this has to be an implicit conversion (or allowed type difference for operators), and not a blanket loss of distinction.

I've now added this to the list of conversions in issue #362.

@dankbaker
Copy link
Contributor Author

What cases did it make a difference? IIRC, the grammar did have some scalars, but they got lost pretty early on. I can't recall a case where it would have made difference, but alas the grammar has undoubtedly changed a little bit since I worked on it.

Intrinsic casting will probably be a sore point for some edge cases. HLSL worked a bit differently. Each intrinsic had a table of acceptable conversions, in a sort of precedent order per intrinsic (it took the first one that passed). And it's completely undocumented. I think building the symbol table is roughly equivalent, but I'm not sure how the name mangling works when there are 2 different possible casts. IIRC, HLSL always went with the first one in it's table.

@ghost
Copy link

ghost commented Aug 10, 2016

At a minimum, float and float1 create different function signatures. For example, this is a valid shader, where if they were identical it would mean redeclaring the function.


void OverloadTest(float1 in0) { }
void OverloadTest(float in0) { }

struct PS_OUTPUT
{
    float4 Color : SV_Target0;
};

PS_OUTPUT main()
{
    PS_OUTPUT psout;
    psout.Color = 1.0;
    return psout;
}

@johnkslang
Copy link
Member

johnkslang commented Aug 10, 2016

That also totally explains the dp() difference between calling with (scalar,vector) vs. (vector1, vector).

Seems pretty regular then to have scalar smear to match vector and longer vector truncate to match shorter vector. I'll code up the shape converter that way.

@johnkslang johnkslang changed the title HLSL: postfix_expression erronously assumes sampler object is a variable scalar == vector is not supported Sep 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants