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

MADs can be obfuscated by clspv #1339

Open
olvaffe opened this issue Apr 30, 2024 · 4 comments
Open

MADs can be obfuscated by clspv #1339

olvaffe opened this issue Apr 30, 2024 · 4 comments

Comments

@olvaffe
Copy link
Contributor

olvaffe commented Apr 30, 2024

clpeak has benchmarks whose kernels resemble

    for(int i=0; i<64; i++)
    {
        x = y * x + y;
        y = x * y + x;
    }

clang optimizes the loop body to

x = (x + 1) * y;
y = (y + 1) * x;

which is still easy enough for vulkan drivers to work out it is two MADs.

But clspv optimizes the loop body to

t = (x + 1);
x = t * y;
y = (x + t) * y;

which is much harder for vulkan drivers to optimize.

Dumping the IR after each pass, the difference is due to the two early InstCombinePass added by clspv.

olvaffe added a commit to olvaffe/clspv that referenced this issue Apr 30, 2024
It prevents InstCombinePass from obfuscating MADs in some cases.
@olvaffe
Copy link
Contributor Author

olvaffe commented Apr 30, 2024

#1340 helps when the data types are scalar. But it does not help when the data types are vectors.

Passing -O0 to clspv does not stop it from adding the two early InstCombinePass too :(

@rjodinchr
Copy link
Collaborator

Yes -O0 is not really used in clspv (#1228 (comment)).

Maybe we should consider adding something in https://github.com/google/clspv/blob/main/lib/UndoInstCombinePass.cpp?

@oscarbg
Copy link

oscarbg commented May 4, 2024

@olvaffe interesting.. can share perf numbers in clpeak scalar tests with and without your patch? to get an idea of expexted perf speedups..

@olvaffe
Copy link
Contributor Author

olvaffe commented May 10, 2024

The perf numbers doubled for short and char. But I also needed to teach mesa to replace (x + 1) * y by x * y + y, for that to be identified as MAD.

It sounds like UndoInstCombinePass.cpp might be a better place to undo the combining. I can certainly look into that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants