Skip to content

[ARM][BUG] Run bad result for vect_type in arm32be #97782

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

Closed
hstk30-hw opened this issue Jul 5, 2024 · 7 comments
Closed

[ARM][BUG] Run bad result for vect_type in arm32be #97782

hstk30-hw opened this issue Jul 5, 2024 · 7 comments

Comments

@hstk30-hw
Copy link
Contributor

hstk30-hw commented Jul 5, 2024

https://godbolt.org/z/vYKsfos5o

typedef short int SV __attribute__((vector_size(16)));
extern void abort(void);

__attribute__((noinline)) void vec_div(SV *x, SV *y)
{
    *x = *y / ((SV){1, 4, 2, 8, 16, 64, 32, 128});
}

SV s[] = {
    ((SV){73, -9123, 32761, 8191, 16371, 1201, 12701, 9999}), 
    ((SV){9903, -1, -7323, 0, -7, -323, 9124, -9199})
};

int main()
{
    SV sr, sr2;
    int i;

    for (i = 0; i < sizeof(s) / sizeof(s[0]); i++) {
        vec_div(&sr, s + i);
        if (sr[0] != s[i][0] / 1 || sr[3] != s[i][3] / 8)
            abort();
        asm volatile("" : : "r"(&sr) : "memory");
    }
    return 0;
}

The expected sr is

[73, -2280, 16380, 1023, 1023, 18, 396, 78]

the result is

[-2280, 73, 1023, 16380, 18, 1023, 78, 396]

I guess rev instruction is misused.

(The vectorize for big endian mode in llvm use rev to handle byte ordering, and introduces many rev instrution, also some bugs.
https://llvm.org/docs/BigEndianNEON.html)

@hstk30-hw hstk30-hw added bug Indicates an unexpected problem or unintended behavior backend:ARM NEON ARM NEON and removed new issue labels Jul 5, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 5, 2024

@llvm/issue-subscribers-backend-arm

Author: None (hstk30-hw)

https://godbolt.org/z/vYKsfos5o
typedef short int SV __attribute__((vector_size(16)));
extern void abort(void);

__attribute__((noinline)) void vec_div(SV *x, SV *y)
{
    *x = *y / ((SV){1, 4, 2, 8, 16, 64, 32, 128});
}

SV s[] = {
    ((SV){73, -9123, 32761, 8191, 16371, 1201, 12701, 9999}), 
    ((SV){9903, -1, -7323, 0, -7, -323, 9124, -9199})
};

int main()
{
    SV sr, sr2;
    int i;

    for (i = 0; i &lt; sizeof(s) / sizeof(s[0]); i++) {
        vec_div(&amp;sr, s + i);
        if (sr[0] != s[i][0] / 1 || sr[3] != s[i][3] / 8)
            abort();
        asm volatile("" : : "r"(&amp;sr) : "memory");
    }
    return 0;
}

The expected sr is

[73, -2280, 16380, 1023, 1023, 18, 396, 78]

the result is

[-2280, 73, 1023, 16380, 18, 1023, 78, 396]

I guess rev instruction is misused.

(The vectorize for big endian mode in llvm use rev to handle byte ordering, and introduces many rev instrution, also some bugs.
https://llvm.org/docs/BigEndianNEON.html)

@llvmbot
Copy link
Member

llvmbot commented Jul 5, 2024

@llvm/issue-subscribers-bug

Author: None (hstk30-hw)

https://godbolt.org/z/vYKsfos5o
typedef short int SV __attribute__((vector_size(16)));
extern void abort(void);

__attribute__((noinline)) void vec_div(SV *x, SV *y)
{
    *x = *y / ((SV){1, 4, 2, 8, 16, 64, 32, 128});
}

SV s[] = {
    ((SV){73, -9123, 32761, 8191, 16371, 1201, 12701, 9999}), 
    ((SV){9903, -1, -7323, 0, -7, -323, 9124, -9199})
};

int main()
{
    SV sr, sr2;
    int i;

    for (i = 0; i &lt; sizeof(s) / sizeof(s[0]); i++) {
        vec_div(&amp;sr, s + i);
        if (sr[0] != s[i][0] / 1 || sr[3] != s[i][3] / 8)
            abort();
        asm volatile("" : : "r"(&amp;sr) : "memory");
    }
    return 0;
}

The expected sr is

[73, -2280, 16380, 1023, 1023, 18, 396, 78]

the result is

[-2280, 73, 1023, 16380, 18, 1023, 78, 396]

I guess rev instruction is misused.

(The vectorize for big endian mode in llvm use rev to handle byte ordering, and introduces many rev instrution, also some bugs.
https://llvm.org/docs/BigEndianNEON.html)

@davemgreen
Copy link
Collaborator

Hi - The last vrev64.32 looks suspicious. There is a call to bitcast here:

return DAG.getNode(ISD::BITCAST, dl, VT, Result);
, that should probably be an nvcast / ARMISD::VECTOR_REG_CAST instead.

@hstk30-hw
Copy link
Contributor Author

Thx bro, the last vrev64.32 should be vrev64.16. I'll fix it.

hstk30-hw added a commit to hstk30-hw/llvm-project that referenced this issue Jul 6, 2024
hstk30-hw added a commit to hstk30-hw/llvm-project that referenced this issue Jul 7, 2024
@hstk30-hw
Copy link
Contributor Author

Fixed it.

@EugeneZelenko EugeneZelenko removed the bug Indicates an unexpected problem or unintended behavior label Jul 8, 2024
@Jolyon0202
Copy link

Hi - The last vrev64.32 looks suspicious. There is a call to bitcast here:

return DAG.getNode(ISD::BITCAST, dl, VT, Result);

, that should probably be an nvcast / ARMISD::VECTOR_REG_CAST instead.

Hi, I have a question. Is there any other BITCAST should be ARMISD::VECTOR_REG_CAST in ARMISelLowering.cpp?

All bitcasts of the vector type need to be converted, is that right?

for example:

return DAG.getNode(ISD::BITCAST, dl, VT, Vmov);

@davemgreen
Copy link
Collaborator

There might well be other places where this is wrong for sure. It shouldn't matter in others though. If the element size is there same then there should not be a differences between the two operations IIRC, and if the value is all zeros (as in getZeroVector), then it would still be all-zeroes after a bitcast/VECTOR_REG_CAST.

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

6 participants