LGTM, with tips :)

I'm not sure I can follow all the consequences of this change, but as for tablegen usage it looks good.

Looks good, thank you!

I think we can submit this.

But is it guaranteed that multiple uses by a single instruction will be adjacent in the list?

Sorry, there is a code for returnining unique instr, so we can use the count:

Yea, I thought there is a guaranty on unique instructions, but looking into defusechain_instr_iterator I don't see how this can be true, so let's continue using the set.

Matt, I'm not against using scan limiter, just about aborting the scan after the last use.

I missed r363675 commit and I think the original semantics of isEXECMaskConstantBetweenDefAndUses should be restored: there should be no scan after the last use.

LGTM. Thank you!

Right, this is hard to follow even for me :). 3rd operand is src1_modifiers, you can use a junk value for this to check whether the DPP combiner don't crash and don't combine it.

I think modifiers are checked correctly by the existing code, but can you add a test for e64 encodings into dpp_combine.mir similar to what is under "check for floating point modifiers" comment?

I'm not sure if e64 instructions have modifiers that cannot be encoded into DPP version, need to check. Otherwise looks good, though I would split typo corrections into separate patch and submit without review.

updated description

ping

updated diff with per review issues done.

Hi, this is the parent revision for the https://reviews.llvm.org/D62401.

• fixed bug when an index was considered in-segment at the segment's end in some cases (lower_bound used instead of upper)
• added fast (binary search) skip for non-containing segments
• improved variable naming

this implementation contains bug with upper segment boundary and misses opportunity to search on sorted segments too, fixing.

replaced std::vector with SmallVector, moved out of the loop.

• replaced output std::vector with template output iterator, return bool on found
• added is_sorted assert
• improved description

split LiveInterval.h change into different patch

LGTM, Thanks!

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: