The DPP combiner pass is enabled since rL353691, https://reviews.llvm.org/rGded96df01e95

Thank you Connor! I really appreciate your effort on this DPP work.

Thanks Nikolai!

rebased diff.

Fixed issue with old = dpp src register when bound ctrl is off.

LGTM, thanks!

Hi Nikolai,

I think we reached the state this can be submitted (and probably enabled with subsequent patch). This would allow any of us make other fixes if required.

Thanks! I wonder how easy is to get confused there. I updated diff with the latest found problems fixed.

In D55444#1352866, @vpykhtin wrote:

Anyways, the other cases look like maybe some other clever optimization for the immediate is hindering this one, for example this with signed minimum:

	v_bfrev_b32_e32 v8, -2                                        ; 7E1058C2
	s_nop 1                                                       ; BF800001
	v_mov_b32_dpp v8, v2  row_bcast:15 row_mask:0xa bank_mask:0xf ; 7E1002FA AF014202
	v_min_i32_e32 v2, v2, v8                                      ; 18041102

Maybe this pass needs to be moved earlier in the pipeline?

I'm not sure I can insert the pass that high, I'll think of how it can be skipped.

Anyways, the other cases look like maybe some other clever optimization for the immediate is hindering this one, for example this with signed minimum:

	v_bfrev_b32_e32 v8, -2                                        ; 7E1058C2
	s_nop 1                                                       ; BF800001
	v_mov_b32_dpp v8, v2  row_bcast:15 row_mask:0xa bank_mask:0xf ; 7E1002FA AF014202
	v_min_i32_e32 v2, v2, v8                                      ; 18041102

Maybe this pass needs to be moved earlier in the pipeline?

Thank you, Connor.

reopening revision.

rebased diff.

Hi Connor,

If there is no strong objections I would like to submit my latest patch here since the pass is disabled and the patch looks better anyway. Otherwise I'll return from NY holidays only on Jan 11.

Fixed issue with identity values and other cases, f32/f16 identity values to be added later.

Connor, indeed, my bad, I'll try to fix this in a couple of days.

updated diff, added tests.

Ok, thank you very much for the review and explanation, I'll try to address it shortly.

Ok, I'll disable it. I'm not sure about 3rd point: are you sayng the pass doesn't actually perform the optimization or it's fundamentally wrong? Because it implemented to handle "identity" cases for add, mul and min/max.

In D55314#1321167, @hakzsam wrote:

Hi,

This change breaks most of the subgroups tests with RADV (ie. dEQP-VK.subgroups.arithmetic.*).

Any reasons why you enabled it by default? Looks like it now triggers a new bug in the AMDGPU backend.

Thanks!

fixed per review issues:

Fixed per review issues.

LGTM. Just a hint: whenever you use "auto X = ..." it's worth to specify explicitly if X is pointer or reference. It's not only saves you from accidental temp object by copy but also makes program easier to read.

I think it looks very good, thank you!

LGTM.

In general adding "custom" code to SelectionDAGBuilder::setValue looks odd. Instead I would add a target-customizable postprocessing loop on pairs of Value <-> SDNode into SelectionDAGISel::SelectBasicBlock right after the DAG is created. The target hook should be able to get whatever it requires LLVM IR analisys and annotate SDNodes.

Added test. It turns out getOccupancyWithLocalMemSize function doesn't account for amdgpu-waves-per-eu attribute so I added call to getWavesPerEU (which does).

fixed per review issues.

LGTM.

SIInstrInfo::doMemOpsHaveSameBasePtr looks generic enough to be default indeed but should we commit this first and then make doMemOpsHaveSameBasePtr default so we could rollback to this one in case of severe regressions? LGTM by the way.

Wow! I wish I had this before! LGTM.

LGTM. However it looks like we should fix TD to follow single naming convention.

Ok, unmangled part looks different indeed. If the issue with pre-link checking is solved this patch is ok with me.

Splitting AMDGPULibFunc in two classes looks a huge overkill. How about modifying AMDGPULibFunc::parse so it could accept unmangled names and just return an enum id for the function (using some fast lookup approach)? Type info for such functions can be left unpopulated and supposed to be handled by the client (as in fold_read_write_pipe).

LGTM.

LGTM.

The implementation of this approach looks good to me. The only question is which way to go to implement v3 vector.

LGTM.

LGTM.

LGTM, Thanks!

LGTM.

LGTM.

LGTM.

LGTM.

LGTM.

LGTM.

LGTM.

LGTM.

LGTM, with nit.

LGTM.

LGTM.

LGTM.