LGTM with another minor round of test reduction

LGTM. I think this is OK, but I think there still might be a more proper way to use the live intervals

Can you try implementing the other approach first, and then applying this on top of it to show the difference more clearly?

In D55539#1327081, @FarhanaAleen wrote:

In D55539#1326636, @arsenm wrote:

Why aren't these matched in the first place? These shouldn't have gotten this far

The offsets are promoted to immediate during the instruction selection if they are 13bit which is the allowed size for globals. This patch is trying to find a 13bit offset from base-address of the nearby instructions by recomputing the relative offset from nearby base address. So, it's more of a global optimization where it needs to traverse the whole program (currently it's limited to the basic block). The optimization also caches all the information to save the compile time. Are you suggesting to do this optimization during instruction selection phase? Wouldn't it be too complex to do during instruction selection?

Why aren't these matched in the first place? These shouldn't have gotten this far

r348801

LGTM with the long line fixed

ping

ping

In D55435#1324753, @arsenm wrote:

The $ is supposed to be for check variables that are used globally and not cleared on a -LABEL

The $ is supposed to be for check variables that are used globally and not cleared on a -LABEL

LGTM

No, it's not committed. Variable + constant is a common case in general.

r348658

In D55179#1323214, @nhaehnle wrote:

Can we also remove the setTruncStoreAction mention of v16i8 from SIISelLowering?

r348625

r348619

r348616

r348615

Move AtomicExpand change, make targets default to cmpxchg

LGTM

I don't really see the point of this and think it will just be an inconvenience to llvm developers.

Add requested comment

In D52416#1320519, @jyknight wrote:

Looks reasonable.

Perhaps add support for pointer types too? Supporting the same set of types for load, store, xchg, and cmpxchg (eventually) would seem sensible.

xxz

In D55301#1321550, @qcolombet wrote:

Hi Matt,

Have you tried to use combined V+S register classes?
By describing such classes, when a S or V register would be split, they would eventually have constraints in that "super" class. Thus, inside of spilling, the splitting mechanism would naturally insert copies of the form [V|S] = copy V+S or V+S = copy [V|S], which seem to be what you are trying to achieve. The advantage of such approach is that we would not have to effectively split the allocation.

Cheers,
-Quentin

I thought mesa was moving to stop using the relocations at all for this?

Added more tests in r348456, use match

I think if we can just declare something simple to follow that doesn't depend on the IR type alignment, we could pack any basic type and align any aggregates to 4

ping

LGTM. Probably should remove the flag from the dpp_combine test

LGTM

Attach correct patch

In D55067#1318959, @rjmccall wrote:

In D55067#1318810, @arsenm wrote:

In D55067#1313419, @rjmccall wrote:

I understand that it's copied into a properly-aligned local variable, but if it affects how the function is called, that's also part of the ABI, and it should be taken from the C alignment, not any vagaries of how struct types are translated into IR.

The other reason to stick with the C alignment value is that it's actually stable across compiler reasons, whereas IRGen does not promise to use the same IR type for a struct across compiler versions.

Now, you're a GPU compiler, so maybe you don't have any ABI requirements, or maybe they're weaker for kernel functions, in which case this is basically irrelevant. But if you do care about compatibility here, you should be aiming to communicate the alignment of this parameter correctly.

This is part of why we largely try to avoid using arbitrary first-class aggregate as parameters in IR.

byval isn't suitable for the purposes of the entry point here, so I don't see what the alternative is with the current constraints. We've defined the ABI input buffer as just the ABI allocation size/alignment of the IR type in order. Clang needs to get this to match what it wants.

What I have said, repeatedly, is that that is not a good ABI rule because the alignment of the IR type is not guaranteed to be meaningful or even stable. Maybe there are good reasons why you don't care about having a stable ABI, but I need you to at least acknowledge that that's what you're doing, because otherwise it is hard for me to approve a patch that I know has this problem.

Rearrange code, apply on top of existing tests

In D55182#1316746, @spatel wrote:

I think this is the right change for IR (I'm trying to do something similar with D50992 but that's been held up while we try to get the backend in shape). .

1. I see that the new code is copying the existing handling of binops around line 315, but can we update this to use 'match()' and reduce the duplication?
2. There should be a test that shows the benefit of using 'isCheapToScalarize()'. IIUC, that would involve a sequence with multiple binops and/or compares between an insertelement and the trailing extract.
3. Please commit the tests with baseline checks as a preliminary commit, so we only show the diffs in this patch.

r348286

r348285

In D55067#1313419, @rjmccall wrote:

I understand that it's copied into a properly-aligned local variable, but if it affects how the function is called, that's also part of the ABI, and it should be taken from the C alignment, not any vagaries of how struct types are translated into IR.

The other reason to stick with the C alignment value is that it's actually stable across compiler reasons, whereas IRGen does not promise to use the same IR type for a struct across compiler versions.

Now, you're a GPU compiler, so maybe you don't have any ABI requirements, or maybe they're weaker for kernel functions, in which case this is basically irrelevant. But if you do care about compatibility here, you should be aiming to communicate the alignment of this parameter correctly.

This is part of why we largely try to avoid using arbitrary first-class aggregate as parameters in IR.