In D99635#2662617, @arsenm wrote:

Missing test

In D99507#2668062, @nhaehnle wrote:

In D99507#2667537, @hliao wrote:

In D99507#2665302, @nhaehnle wrote:

I think this requires a lot more thought.

+1

What I'd like to know: why are we reloading a lane mask via V_READFIRSTLANE in the first place? I would expect one of two types of reload:

1. Load from a fixed lane of a VGPR using V_READLANE.

That depends on how we spill a SGPR by writing a fixed lane or write an active lane. The 1st one, without saving/restoring, we will overwrite the live values in the inactive lanes. HPC workloads are hit by that issue and cannot run correctly.

Let me rephrase to make sure I understood you correctly. You're saying that spilling an SGPR to a fixed lane of a VGPR may cause data of an inactive lane to be overwritten. This is a problem if the spill/reload happens in a called function, because VGPR save/reload doesn't save those inactive lanes. (HPC is irrelevant here.)

In D99507#2665302, @nhaehnle wrote:

I think this requires a lot more thought.

+1

What I'd like to know: why are we reloading a lane mask via V_READFIRSTLANE in the first place? I would expect one of two types of reload:

1. Load from a fixed lane of a VGPR using V_READLANE.

the case is no longer valid considering concurrent kernel execution.

It seems to me that we may need to revise CFG lowering to avoid updating EXEC directly and later revise it based on whether the restoring mask needs reloading or not. Here's the brief thought in my mind:

• Instead of lowering CFG early before RA, lower it after RA. As a byproduct, it also remove the need of "terminator" version of exec mask manipulation instructions.
• When CFG is being lowered, it could update EXEC eagerly if the merge point doesn't need to reload the mask; Otherwise, it just needs to translate as what we currently did.

In D99507#2656766, @arsenm wrote:

I'm not comfortable adding a pass to fixup a bug in control flow lowering. I think we just need to actually try to model divergent predecessors/successors explicitly

This's the companion fix for D96980. That explain the myth why the original agnostic SGPR spill/reload is proposed to solve the issue SGRP spill/reload may be executed when exec mask goes to zero.
We did try to skip executing code when exec mask goes to zero by branch on EXECZ (to the target block) or EXECNZ (to the fallthrough block.) We may run instructions with zero exec mask. But, that's usually not an issue as we immediately restore the exec mask on the targeted block. Code following that exec mask restoration won't be executed in 0 mask. However, if that mask restoration needs to reload a spilled exec mask, we will run the SGPR reload with 0 mask, where v_readfristlane has undefined behavior when exec mask is zero.
This patch tries to mitigate that case by not evaluating exec mask that early or clearing the exec mask when the branch target has mask restoration following SGRP reload. Instead of checking EXECZ or EXECNZ, the exec mask evaluation is duplicated with a temporary SGRP as the destination (without updating exec mask directly), checking SCC0 is equivalent to EXECZ. Exec mask is only evaluated when the result won't be zero. For instance,

Update tests with more atomic ops.

Update summary.

In addition, we already heavily use v_readfirstlane in our codegen due to some patterns benefits by using vector instructions when no corresponding scalar instructions could be used. I believed it's quite safe that it's guaranteed v_readfirstlane won't be executed when exec mask goes to 0.

Remove that unnecessary change and add rationale why that's safe for the original concerns.

In D96517#2578930, @sebastian-ne wrote:

In D96517#2578884, @hliao wrote:

why exec mask = 0 case is a valid one, won't we already branch away once exec mask goes to zero?

That is the question it comes down to. If it is guaranteed that exec is never 0, i.e. at least one bit is always set, I’m in favor of your patch.

To have some numbers, I saw some functions spilling 30 SGPRs to scratch, so it can be more than just a one or two.

Mark SI_SPILL_<n>_RESTORE having unwanted effect so that they would be executed under exec mask 0.

In D96517#2578930, @sebastian-ne wrote:

In D96517#2578884, @hliao wrote:

why exec mask = 0 case is a valid one, won't we already branch away once exec mask goes to zero?

That is the question it comes down to. If it is guaranteed that exec is never 0, i.e. at least one bit is always set, I’m in favor of your patch.

To have some numbers, I saw some functions spilling 30 SGPRs to scratch, so it can be more than just a one or two.

In D96980#2576062, @arsenm wrote:

In D96980#2575930, @ruiling wrote:

In D96980#2574913, @sebastian-ne wrote:

I think this approach fails when exec is zero.
The v_mov for the save will be a noop, the v_readfirstline for the restore will read lane 0, which contains some unknown value.

For exec == 0 when reloading, I think the basic block that contains v_readfirstlane will be jumped over, see SIInsertSkips.cpp and hasUnwantedEffectsWhenEXECEmpty()

I'm trying to eliminate SIInsertSkips. Initially, all branches that go over exec changes should insert the skip jump. We then should eliminate them in cases where they aren't needed and the blocks are short.

In D96980#2576128, @ruiling wrote:

In D96980#2576062, @arsenm wrote:

In D96980#2575930, @ruiling wrote:

In D96980#2574913, @sebastian-ne wrote:

I think this approach fails when exec is zero.
The v_mov for the save will be a noop, the v_readfirstline for the restore will read lane 0, which contains some unknown value.

For exec == 0 when reloading, I think the basic block that contains v_readfirstlane will be jumped over, see SIInsertSkips.cpp and hasUnwantedEffectsWhenEXECEmpty()

I'm trying to eliminate SIInsertSkips. Initially, all branches that go over exec changes should insert the skip jump. We then should eliminate them in cases where they aren't needed and the blocks are short.

I realized SIRemoveShortExecBranches.cpp has done correctness checks. So removing SIInsertSkips should work if we make sure there will always be a branching instruction for each control flow change.

In D96517#2574435, @arsenm wrote:

I think once we're spilling SGPRs to memory with these tricks, you've given up on performance. We just need *anything* that works, and put effort into guaranteeing we never need to do this (aside from the degenerate inline-asm case using all VGPRs). I'm not worried about getting the ideal SGPR-to-memory sequence

why exec mask = 0 case is a valid one, won't we already branch away once exec mask goes to zero?

In D96980#2574913, @sebastian-ne wrote:

I think this approach fails when exec is zero.
The v_mov for the save will be a noop, the v_readfirstline for the restore will read lane 0, which contains some unknown value.
exec=0 is a corner case, but I don’t think we can build on that.

Other than that, it surely is more efficient than the (worst case) 4 memory operations introduced by saving all lanes of the used VGPR :)

In D96980#2573982, @foad wrote:

HPC workloads mostly spill 1 or 2 SGPRs.

Can you explain this a bit more? Spilling SGPRs to memory is supposed to be a very rare case. Why is it common for HPC workloads? Is there a better way to fix it?

In D96980#2574001, @critson wrote:

Is it at all possible to encapsulate your failure case in a lit test?

Is the problem not with the exec manipulation, but really that the register scavenger choosing an inappropriate register, w.r.t. wave level CFG?

In D96980#2572918, @arsenm wrote:

Conflicts with D96336/D96517

Shall we optimize the cases where only 1 or 2 SGPRs are to be spilled or reloaded when there's a VGPR scavenged? In this case, we only need one or two loads/stores to spill/reload that SGPR. From the number of LD/ST, that original one (based on broadcast and v_readfirstlane) is still OK but we need less code. Also, there's no latency due to the restore of that restore of that tmp VGPR.

In D96980#2572918, @arsenm wrote:

Conflicts with D96336/D96517

@rsmith, @rnk and @rjmccall, could you review the latest change addressing the memory usage issue?

PING for review

Revise comments.

• Remove DeviceManglingNumber to reduce memory usage.
• Add a table in ASTContext for device lambda mangling numbers if that lamba requires.

Rebase to the main branch.

PING for review.

Rebase

Fix typo.

Forget that C function could be overloaded on Clang with overloadable
extension. With that, we don't need to mark functions from <ymath.h> as HD.
Instead, we could provide their device-side implementation directly.

Only mark HD attributes in ymath.h wrapper header when compiled with MSVC.

Revise following reviewers' comments.

PING

PING

Fix the cmake to distribute that header wrapper.

These functions are pure C functions.

In D93638#2466097, @jdoerfert wrote:

Disclaimer: I request changes because of the next sentence, other than that I have no objection but also cannot review this.
All cuda_wrapper headers say something about complex in the first row, copy & paste error. All have the wrong license text (I think).

Fix license.

Fix typo again.

Fix typo.

Beyond the enabling of the compilation with <complex> on Windows, I really have the concern on the current approach supporting <complex> compilation in the device compilation. The device compilation should not relies on the host STL implementation. That results in inconsistent compilation results across various platforms, especially Linux vs. Windows.
BTW, the use of <complex> in CUDA cannot be compiled with NVCC directly even with --expt-relaxed-constexpr, c.f. https://godbolt.org/z/3f79co

The build is broken due to the missing file.