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

LLVM-MCA thinks that GCC's vectorization of loop reduction for Zen3 is ~50% faster #121344

Open
TiborGY opened this issue Dec 30, 2024 · 11 comments

Comments

@TiborGY
Copy link

TiborGY commented Dec 30, 2024

I am trying to optimize a few hotspots in a larger body of code, one of which is an RMSD computation implemented as loop reduction, where the loop bounds are completely known at compile time:

double RMSD(const double* const dm1, const double* const dm2){
    const size_t distsPerGeom = (12*11)/2;
    double RMSD = 0.0;
    for (size_t i=0; i<distsPerGeom; i++){
        RMSD += std::pow(dm1[i] - dm2[i], 2);
    }
    return std::sqrt(RMSD / distsPerGeom);
}

What I have found, is that according to LLVM-MCA, GCC is far better at vectorizing this function, 2823 total cycles vs. 4455, see this link:
https://godbolt.org/z/q85rq3bW4

I have not tested this on real HW yet, but I cannot see how this is not indicative of some issue in an LLVM component. Either MCA's cycle counts are wrong for Zen3, or clang is not as good at vectorizing as GCC is.

@RKSimon
Copy link
Collaborator

RKSimon commented Dec 30, 2024

It appears to be a reassociation issue - clang is summing RMSD into the same ymm7 accumulation result:

vfmadd231pd %ymm2, %ymm2, %ymm7
vfmadd231pd %ymm1, %ymm1, %ymm7
vfmadd231pd %ymm0, %ymm0, %ymm7

which means that all the vfmadd231pd instructions are performed serially, while gcc is splitting the FMA accumulation and then adding the sub-results together, improving IPC.

vfmadd231pd %ymm0, %ymm0, %ymm1
..
vfmadd132pd %ymm0, %ymm4, %ymm0
..
vfmadd231pd %ymm0, %ymm0, %ymm2
..
vaddpd %ymm2, %ymm0, %ymm0
vaddpd %ymm1, %ymm0, %ymm0

@llvmbot
Copy link
Member

llvmbot commented Dec 30, 2024

@llvm/issue-subscribers-tools-llvm-mca

Author: None (TiborGY)

I am trying to optimize a few hotspots in a larger body of code, one of which is an RMSD computation implemented as loop reduction, where the loop bounds are completely known at compile time: ``` double RMSD(const double* const dm1, const double* const dm2){ const size_t distsPerGeom = (12*11)/2; double RMSD = 0.0; for (size_t i=0; i<distsPerGeom; i++){ RMSD += std::pow(dm1[i] - dm2[i], 2); } return std::sqrt(RMSD / distsPerGeom); } ``` What I have found, is that according to LLVM-MCA, GCC is far better at vectorizing this function, 2823 total cycles vs. 4455, see this link: https://godbolt.org/z/ov45ebsoM

I have not tested this on real HW yet, but I cannot see how this is not indicative of some issue in an LLVM component. Either MCA's cycle counts are wrong for Zen3, or clang is not as good at vectorizing as GCC is.

@firewave
Copy link

FYI The data might be tainted. llvm-mca will default to the underlying CPU and since this is a cloud service it will obviously have various possible platforms. So it is possible to get different results for the same input. If you add -mcpu=generic (or your intended target) to the "Arguments" you will get consistent results.

See compiler-explorer/compiler-explorer#4085.

@RKSimon
Copy link
Collaborator

RKSimon commented Dec 30, 2024

Adding an explicit -mcpu=znver3 to the llvm-mca command lines made very little difference to the estimated instruction cycles, and still shows the 2x ratio in the "Total Cycles" numbers.

@mshockwave
Copy link
Member

FYI The data might be tainted. llvm-mca will default to the underlying CPU and since this is a cloud service it will obviously have various possible platforms. So it is possible to get different results for the same input. If you add -mcpu=generic (or your intended target) to the "Arguments" you will get consistent results.

See compiler-explorer/compiler-explorer#4085.

It is true that one almost certain wants to add an explicit -mcpu= option when using llvm-mca, but I don't think -mcpu=generic is a good one since IIRC it's effectively an alias of -mcpu=sandybridge -- a processor that is too old (of course it still depends on which kind of applications are you profiling)

@firewave
Copy link

Adding an explicit -mcpu=znver3 to the llvm-mca command lines made very little difference to the estimated instruction cycles, and still shows the 2x ratio in the "Total Cycles" numbers.

In this case it indeed did not make a different and the underlying architecture is identical. It might be possible that using a -mcpu on the compiler tab will also apply this to the llvm-mca invocation somehow (maybe what was suggested in the comment in my ticket was actually applied). I never used that flag and ended up with Zen vs. Skylake output (as outlined in the ticket).

@TiborGY
Copy link
Author

TiborGY commented Dec 30, 2024

I have looked at the MCA outputs before I have submitted this issue, and both showed that MCA used znver3 by default, so the data were valid.
But good catch, if MCA defaults to the host CPU it might drift to whatever cloud machine happens to be free at the moment, so I have updated the link with explicit znver3: https://godbolt.org/z/q85rq3bW4

@mshockwave
Copy link
Member

It appears to be a reassociation issue - clang is summing RMSD into the same ymm7 accumulation result:

vfmadd231pd %ymm2, %ymm2, %ymm7
vfmadd231pd %ymm1, %ymm1, %ymm7
vfmadd231pd %ymm0, %ymm0, %ymm7
which means that all the vfmadd231pd instructions are performed serially, while gcc is splitting the FMA accumulation and then adding the sub-results together, improving IPC.

vfmadd231pd %ymm0, %ymm0, %ymm1
..
vfmadd132pd %ymm0, %ymm4, %ymm0
..
vfmadd231pd %ymm0, %ymm0, %ymm2
..
vaddpd %ymm2, %ymm0, %ymm0
vaddpd %ymm1, %ymm0, %ymm0

Assuming this is to be solved in MachineCombiner (though preferably it should be solved in ReassociatePass on the pre-vectorized scalar code), I think this can be solved by adding new patterns into X86InstrInfo::getMachineCombinerPatterns

@RKSimon
Copy link
Collaborator

RKSimon commented Dec 30, 2024

I'd much prefer to see this handled in the middle end and not make it x86 specific

@mshockwave
Copy link
Member

I'd much prefer to see this handled in the middle end and not make it x86 specific

This seems to be caused by the fact that ReassociatePass is ran before LoopUnroll. Because not until LoopUnroll do we see the long critical path consisted of fadd + fmul.

@TiborGY
Copy link
Author

TiborGY commented Jan 2, 2025

This seems to be caused by the fact that ReassociatePass is ran before LoopUnroll. Because not until LoopUnroll do we see the long critical path consisted of fadd + fmul.

* Before ReassociatePass: https://godbolt.org/z/feGPb9bqr

* After LoopUnroll: https://godbolt.org/z/hqnqnjvqb

This is probably a dumb idea as I am not too familiar from the internals of LLVM, but could the ReassociatePass be simply moved to run after LoopUnroll? Or if that regresses optimiztation for something else, run ReassociatePass a second time after LoopUnroll?

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