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

Possible bug when writing the vertex fetch instructions #27

Closed
godlikepanos opened this Issue Apr 11, 2018 · 9 comments

Comments

Projects
None yet
3 participants
@godlikepanos
Copy link

godlikepanos commented Apr 11, 2018

Hi,

I have a vertex attribute with format equal to VK_FORMAT_A2B10G10R10_SNORM_PACK32. My program fails on pipeline creation. Did some digging and I hit an assertion deep into LLVM code.

The issue starts in:

llpcVertexFetch.cpp:1163

// Insert alpha channel: %vf0 = insertelement %vf0, %a, 3
vertexFetch[0] = InsertElementInst::Create(vertexFetch[0],
pAlpha,
ConstantInt::get(m_pContext->Int32Ty(), 3),
"",
pInsertPos);

The pAlpha is of type FloatTyID and vertexFetch[0] is VectorTyID.

Later on there is this assert:

assert(isValidOperands(Vec, Elt, Index) &&
"Invalid insertelement instruction operands!");

which states that "Second operand of insertelement must be vector element type" which is probably not.

If you do a quick code inspection, is this piece of code correct? If yes I'll dig some more and create a repro case.

@linqun

This comment has been minimized.

Copy link
Collaborator

linqun commented Apr 12, 2018

It seems it is a bug in our driver. vertexFetch[0] shouldbe VectorTyID with integer type. but pAlpha is float now. please try to change the type to Int32Ty in previous bitcast instruction

// %a = bitcast %a to i32
pAlpha = new BitCastInst(pAlpha, m_pContext->Int32Ty(), "", pInsertPos);

please let me know if it can work in your case.

@godlikepanos

This comment has been minimized.

Copy link
Author

godlikepanos commented Apr 12, 2018

The proposed change fixed my issue. Feel free to close the ticket now or when you commit the fix. Thanks for the fast response.

@godlikepanos

This comment has been minimized.

Copy link
Author

godlikepanos commented Apr 21, 2018

After some more digging it seems something is still a bit off.

As I mentioned before I use VK_FORMAT_A2B10G10R10_SNORM_PACK32 (for tangents where w component holds the sign of the bitangent). I've done an experiment where I hardcode 0b11 to the alpha component of this format when building my assets. In theory, that will return -1.0 inside the vertex shader. Nevertheless the shader reports +1.0.

In Mesa GL and nVidia GL the behaviour seems the expected. In AMDVLK and -oddly enough- in RADV it seems it doesn't.

By looking at your comments in llpcVertexFetch.cpp I see that there are some annoyances around the sign of alpha.

@linqun

This comment has been minimized.

Copy link
Collaborator

linqun commented Apr 25, 2018

sorry for delay response, I agree this code is too tricky. it is easy to cause bugs after LLVM optimization.

Can you try following code in "else if (pFormatInfo->nfmt == BUF_NUM_FORMAT_SNORM)"

pAlpha = new BitCastInst(pAlpha, m_pContext->FloatTy(), "", pInsertPos);
pAlpha = BinaryOperator::CreateMul(pAlpha,
ConstantFP::get(m_pContext->FloatTy(), 3.0f),
"", pInsertPos);
auto pCond = new ICmpInst(pInsertPos,
CmpInst::FCMP_UGT,
pAlpha,
ConstantFP::get(m_pContext->FloatTy(), 1.0f),
"");
pAlpha = SelectInst::Create(pCond, ConstantFP::get(m_pContext->FloatTy(), -1.0f), pAlpha, "", pInsertPos);
pAlpha = new BitCastInst(pAlpha, m_pContext->Int32Ty(), "", pInsertPos);

ps: above code isn't tested yet. please let me know if doesn't work.

@godlikepanos

This comment has been minimized.

Copy link
Author

godlikepanos commented Apr 25, 2018

I'm afraid that asserts in:

pAlpha = BinaryOperator::CreateMul(pAlpha, ConstantFP::get(m_pContext->FloatTy(), 3.0f), "", pInsertPos);

@linqun

This comment has been minimized.

Copy link
Collaborator

linqun commented Apr 26, 2018

Got it, I will write a test to test it and let you know when it is done.

@linqun

This comment has been minimized.

Copy link
Collaborator

linqun commented Apr 26, 2018

please try again, thanks!

        else if (pFormatInfo->nfmt == BUF_NUM_FORMAT_SNORM)
        {
            // %a = bitcast %a to f32
            pAlpha = new BitCastInst(pAlpha, m_pContext->FloatTy(), "", pInsertPos);
            // %a = mul %a, 3.0f
            pAlpha = BinaryOperator::CreateFMul(pAlpha,
                                               ConstantFP::get(m_pContext->FloatTy(), 3.0f),
                                               "",
                                               pInsertPos);
            // %cond = ugt %a, 1.5f
            auto pCond = new FCmpInst(pInsertPos,
                                      FCmpInst::FCMP_UGT,
                                      pAlpha,
                                      ConstantFP::get(m_pContext->FloatTy(), 1.5f),
                                      "");
            // %a = select %cond, -1.0f, pAlpha
            pAlpha = SelectInst::Create(pCond,
                                        ConstantFP::get(m_pContext->FloatTy(), -1.0f),
                                        pAlpha,
                                        "",
                                        pInsertPos);
            // %a = bitcast %a to i32
            pAlpha = new BitCastInst(pAlpha, m_pContext->Int32Ty(), "", pInsertPos);
        }
        else if (pFormatInfo->nfmt == BUF_NUM_FORMAT_SSCALED)
        {
           // %a = bitcast %a to float
            pAlpha = new BitCastInst(pAlpha, m_pContext->FloatTy(), "", pInsertPos);
            // %a = fptosi %a to i32
            pAlpha = new FPToSIInst(pAlpha, m_pContext->Int32Ty(), "", pInsertPos);
            // %a = shl %a, 30
            pAlpha = BinaryOperator::CreateShl(pAlpha,
                                               ConstantInt::get(m_pContext->Int32Ty(), 30),
                                               "",
                                               pInsertPos);
            // %a = ashr a, 30
            pAlpha = BinaryOperator::CreateAShr(pAlpha,
                                                ConstantInt::get(m_pContext->Int32Ty(), 30),
                                                "",
                                                pInsertPos);
            // %a = sitofp %a to float
            pAlpha = new SIToFPInst(pAlpha, m_pContext->FloatTy(), "", pInsertPos);
            // %a = bitcast %a to i32
            pAlpha = new BitCastInst(pAlpha, m_pContext->Int32Ty(), "", pInsertPos);
@godlikepanos

This comment has been minimized.

Copy link
Author

godlikepanos commented Apr 26, 2018

Thanks again. Now it appears correct.

@jinjianrong

This comment has been minimized.

Copy link
Member

jinjianrong commented May 10, 2018

The fix is included May 9 code drop. Please check it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.