LGTM

In D47370#1112729, @rampitec wrote:

Are we sure that is what RT(s) do?

Needs a test, preferably the full set of AA checks with 32 bit constant

In D46298#1110788, @alex-t wrote:

I agree isVGPR should be illegal to call for R600

So, how would you handle the bunch of r600 tests then?
We could claim the support of DA related stuff for GCN only but it should be explicitly agreed and documented somehow.

In D46298#1110597, @tstellar wrote:

In D46298#1086262, @alex-t wrote:

Could you please clarify - why do you consider that check meaningless for r600?
I see that this line : " const SISubtarget &ST = MF->getSubtarget<SISubtarget>(); " is misleading and in fact is not correct.
I'd better check and choose the R600Subtarget or SISubtarget.
If I understand right we need just check which subtarget to retrieve for physregs check.

Since we consider any VGPR formal argument as divergent it does not matter R600 or SI at all.
We need to choose right TargetRegisterInfo (r600 or SI again)

So, for virtual register : if (MRI.isLiveIn(Reg) && TRI.isVGPR(Reg) ) return true

Or I maybe don't know something substantial? :)

I would expect the TRI.isVGPR() call to crash if this was called from an R600 code path, so that made me think this was dead code.

LGTM

LGTM

Needs a test, preferably the full set of AA checks with 32 bit constant

LGTM. I don't believe that there are reasons RPO doesn't work other than bugs in the structurizer. The sorting used now is clearly broken, and if this fixes the concept it was going for this is a step in the right direction

LGTM

r333019

Fix missing test update

In D47207#1108244, @hliao wrote:

please add regression tests

r332953

LGTM

LGTM

LGTM

LGTM

LGTM

r332771

I think the intent of the current code is for the address space to correspond to a "target address space" as if the user code used attribute((address_space(n))) to specify a pointer value. This is confusingly named, and different from the target address space selected for a LangAS. I think we need to add some mechanism for specifying the builtin is a LangAS ID. Since Ideally this would also work for multiple languages (e.g. cuda_constant or opencl_constant for the same builtin) I think there needs to be some callback triggered for the address space value. This possibly needs to be distinct from the current pointer descriptor to avoid breaking the possibility of user defined address spaces. There aren't really any other users of builtins with address spaces. NVPTX has some, but the tests seem to not actually try to use the declared address space and pass generic pointers to them.

ping

I'm looking at how the address space mapping works for builtins, and I think what's there is just uselessly broken and needs to be fixed. It seems to be operating under the assumption that the address spaces the target defines are totally disjoint from the language address spaces

In D46754#1104799, @arsenm wrote:

In D46754#1104736, @timcorringham wrote:

Corrected the ordering of operands to interp_p2_f16, added lowered
intrinsics to list of those that cware a source of divergence, and
amended LIT test.

I have not overloaded the intrinsics as I don't believe it is possible
in this case as they have an additional operand, and apart from that
additional operand the interp_p1_f16 has the same types as the 32 bit
version, so there are no type differences to provide disambiguation.

Is the extra parameter you're referring the high parameter to change where the register is read from the high or low bits? That shouldn't be exposed in the intrinsic at all. Eliminating the high bit extraction is a codegen optimization pattern

In D46754#1104736, @timcorringham wrote:

Corrected the ordering of operands to interp_p2_f16, added lowered
intrinsics to list of those that cware a source of divergence, and
amended LIT test.

I have not overloaded the intrinsics as I don't believe it is possible
in this case as they have an additional operand, and apart from that
additional operand the interp_p1_f16 has the same types as the 32 bit
version, so there are no type differences to provide disambiguation.

Account for difference between 1.2 and 2.0 wrt uniform-work-group-size

Also handle -cl-uniform-work-group-size attribute

In D40405#1102009, @rampitec wrote:

In D40405#1100698, @arsenm wrote:

Test should go in test/LoopStrengthReduce/AMDGPU and run just LSR

I can move the test, but I cannot reproduce the bug with opt.

What is the test actually testing? I don't think there should be any difference from this in the floor lowering, and I don't really want a separate test file duplicating all of the lowering testing

r332453

Test should go in test/LoopStrengthReduce/AMDGPU and run just LSR

I still think this process of using the LoopDepth and sorting from anything other than RPO is fundamentally broken, but if everything is passing with this that's an improvement.

Also f64 tests?

Add more asserts

LGTM with the check lines enhanced

This needs a test (I'm pretty sure it already exists and should break with this change).

LGTM

r332196

r332195

LGTM

LGTM. At this point I would be more interested in what source and dest modifier folding looks like in global isel

r331919

r331917

r331916

r331909

r331906