Differential D152649
[AMDGPU] Enable Atomic Optimizer and Default to Iterative Scan Strategy. Authored by pravinjagtap on Jun 11 2023, 7:47 AM.
Details The D147408 implemented new Iterative approach for scan computations The changeset https://github.com/GPUOpen-Drivers/llpc/pull/2506 This patch enables atomic optimizer pass and selects Iterative
Diff Detail
Event Timeline
Comment Actions Code LGTM. I'm not sure how far we want to go with disabling this optimization in lit tests that are not specifically testing the optimization itself.
Comment Actions LGTM, with or without changing the default strategy.
Comment Actions Here's the corresponding LLPC patch: https://github.com/GPUOpen-Drivers/llpc/pull/2523 Comment Actions This patch seems to have had catastrophic results on the build times in the libc project. See the buildbot where the build times have gone from ~15 minutes to over an hour https://lab.llvm.org/staging/#/builders/247. Reverting this patch causes the time for my to build a test to go from 55 seconds to 5 seconds. I can try to get pass timing later. Comment Actions If this is only affecting AMDGPU and libc, is it a problem? This optimization is very important. Of course, it would be great if it can be sped up. Comment Actions I don't know if it's just a libc problem, it probably just highlights the problem since it's a considerably large application with a good number of atomic operations thanks to RPC calls. Maybe @ye-luo can let me know if he's had any compile time regressions recently. In any case a ~10x increase in link time is a little steep for a single optimization. I can always disable this pass in the libc build but I think that other large applications are going to see similar changes so we need to think of a way to do this that isn't so slow. Comment Actions The newly added assertions at https://github.com/llvm/llvm-project/blob/1ebbbf1614cfdbf6d78f4f2a665cdea9cbb2beb8/llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp#L774 and https://github.com/llvm/llvm-project/blob/1ebbbf1614cfdbf6d78f4f2a665cdea9cbb2beb8/llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp#L802 during D147408 seems to be causing this problem. Verifying dominator tree is very expensive using assertions like this. The alternative to this check is -verify-dom-info which seems to be broken at this moment. Let me investigate. The time for running libc tests is recovered without these assertions. Comment Actions From the pass, / This pass optimizes atomic operations by using a single lane of a wavefront Could someone expand on this? The pass seems to take an atomic operation that lowers to a single instruction and replace it with a loop over active lanes, each of which calls that same instruction. How/when is that better? Comment Actions
No - it takes an atomic operations that is executed by (we assume) many lanes, and replaces it with an atomic that is only executed by a single lane, because it is inside some kind of "if (laneid==0)" check. To make this work you might have to fettle the inputs or outputs of the atomic op, to make it work "as if" it was executed many times by many lanes. E.g. for an atomic add you have to do a plus-reduction of the inputs to the many-lane atomic adds, to get the value to pass into the single-lane atomic add. That's where the loop comes in: it is one way of calculating the plus-reduction. But since it is only doing ALU work, it is still supposed to be better than running a whole bunch of serialised atomic memory operations. Comment Actions I only have logs for the Kokkos build handy: this optimization was the second most expensive in total time with ~17% share. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
The old option can be dropped, none should be a value for the strategy