• Relocate tests and add extra tests for parsing errors.
• Enhance the parser to report any missing definitions on forward referenced metadata.

PING for the further comment on the round-trip test, which requires MIR parser support reviewed @ D103282

Fix typos.

Only apply that abs/flip on fptosi for f32.

• Add global-isel support.
• Revise the method name following the suggestion.

In D103205#2811695, @arsenm wrote:

Needs standalone MIR tests (i.e. round trip tests not bundled in a unit test)

Also I would like to see some tests where there are inconsistencies between the metadata present in the IR section, and the explicit metadata in the MIR

Kindly PING again

Add 'amdgpu-' prefix in that option.

This's the short-term solution until all issues exposed after the frontend change are fixed.

Kindly PING

Fix warngins from clang-tidy.

Rebase

Rebase

In D102255#2785665, @hliao wrote:

Clean up after splitting MIR printer and parser changes.

the patch is revised following comments.

Separate that test into MIR.

Clean up after splitting MIR printer and parser changes.

Revise that machine tracker a bit to ensure machine metadata is alway per-function state.

In D102255#2780461, @hliao wrote:

In D102255#2780327, @nikic wrote:

Can you please split off the metadata parsing part into a separate patch? It's not directly related to the memcpy lowering.

OK, I need to add unit tests to verify that as we won't be able to generate machine metadata within the current backend. I will prepare that this night.

In D102255#2780327, @nikic wrote:

Can you please split off the metadata parsing part into a separate patch? It's not directly related to the memcpy lowering.

Rebase.

Rebase on the pre-committed test D103106.

Move the AAInfo preparation into the code lowering memcpy/memmove/memset.

PING

PING

PING for review

Just read the relevant threads and bugs reported on the change of allowing exact-overlap on llvm.memcpy. See the reference list at the end. Personally, I think it's OK to assume NoAlias added here. By allowing exact-overlap in llvm.memcpy, the most significant change is on the basic-aa, which must consider the case where the source and destination of llvm.memcpy is the same. The make senses at the IR level, where llvm.memcpy is treated as a single op as the exact-overlap means the copy is a no-op and won't always overwrite the destination memory. But, where we lower that copy into loads/stores, we say that loads/stores won't alias. That's fine as the order between loads and stores on the same offset (or location for the exact-overlap case) is established through the data dependency. In addition, the no-alias established here is a scoped one, which only applies to loads/stored from this llvm.memcpy only. It won't affect the AA result between them to loads/stores out of the scope. (This patch depends on https://reviews.llvm.org/D102215, which propagates scoped AA on mem ops into loads/stores after lowering.)

In D102255#2766721, @jeroen.dobbelaere wrote:

In D102255#2765788, @jeroen.dobbelaere wrote:

In D102255#2755372, @nikic wrote:

I agree that rescheduling the loads/stores is correct even if src and dst are equal. However, the metadata itself is still incorrect: It will claim that the loads/stores are NoAlias, even though they are actually MustAlias.

The proposed version introduces 2 scopes, assigning all store to one scope and all loads to another.

I order to avoid the MustAlias vs NoAlias problem, we could introduce a scope per load-store pair. Of course, that might be a lot of extra scopes..

This was discussed earlier today in LLVM's AA Technical call. This method should work, but one possible gotcha came up: sometimes a memcpy lowering results in overlapping load/stores. Those overlapping load/stores must remain 'aliasing', so they should belong to the same scope.

possible example:

// memcpy(dst, src, 23) becomes:
store i64 dst+0, (load i64 src+0)    // scope 0
store i64 dst+8, (load i64, src+8)   // scope 1, not overlapping with previous pair
store i64 dst+15, (load i64, src+15) // also scope 1, overlapping with previous pair

Add MIR printer and parser support for scoped AA metadata generated in the backend.

Revise the AA metadata matching.

Include tests examining the MIR to ensure scoped AA metadata are attached on loads/stores lowered from mem ops.

Good catch! LGTM!

In D102255#2752039, @jeroen.dobbelaere wrote:

It does look like a valid way to indicate that the individual loads and stores are independent, except for their value dependency.

Given the very late introduction of new !alias.scope and !noalias metadata, is their a way to have a testcase look at a machine ir dump, together with the metadata output at that phase ?

In D102255#2751896, @nikic wrote:

Do I understand correctly that this patch is trying to claim that the memcpy src and dst do not alias through scoped alias metadata? If so, I'm afraid this is incorrect. Our contract for llvm.memcpy requires that src/dst are either NoAlias or MustAlias (but not PartialAlias). From LangRef:

The ‘llvm.memcpy.*’ intrinsics copy a block of memory from the source location to the destination location, which must either be equal or non-overlapping.

Add more tests and revise following the comment.

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.