In D87882#2282278, @rampitec wrote:

Doesn't loop block self dominate?

tests moved to existing rotl/rotr tests

Tests added. ROTR case added.

In D87107#2261429, @rampitec wrote:

In D87107#2259217, @alex-t wrote:

The idea is:

For the block that is queried

1. Look for it's predecessors that can pass control through the S_EXECZ/EXECNZ
2. If found one, look for exec restoring code starting the beginning of the block being queried.
3. Since exec restoring code always belong to the block prologue, search the prologue and if not found return false.

Considering your comment that exec == 0 does not matter, we'd rather search upwards before the immediate dominator block in encountered to check what we met first - exec modify or exec restore. The problem here is that XOR can be both.

Thanks. More or less you want to disable split in an empty block preceded by c_branch_exec[n]z. I can understand why this would be a problem. In reality such block can be either empty or contain another branch, because it does not make any sense to pass a control to a block with vector instructions and have no active lanes.

But this leaves couple problems:

1. It does not disallow a split in a block prologue before exec is restored. This creates exactly the same problem.
2. It does not disallow a split even in an empty block where EXEC is not zero, but just wrong. The problem is not zero EXEC, the problem is wrong EXEC, zero is just once case of this.

JBTW, even with all of this it is still OK to split an LI of SGPR. I'd say at the very least callback needs to take an LI in question as well.

Enhanced PSDB passed

The idea is:

In D87107#2256913, @rampitec wrote:

In D87107#2256864, @alex-t wrote:

Also I still think that disabling a whole "endif" block is an overkill.

It only is disabled if S_OR_B64 exec, ... is in the middle of the block that should never happen.

while (isBasicBlockPrologue(*J)) {
  if (IsExecRestore(&*J))
    return true;

assumes that if exec is restored in the block prologue it is valid

So practically it never happens and split is effectively only disabled in an empty block? I said it already: it does not matter that exec is zero, what matters is that it does not match. It does not matter that a block is empty as well, it is enough to split before s_or to hit the bug.

Also I still think that disabling a whole "endif" block is an overkill.

So, since we now have sensible diff to discuss...
Why I decided to disallow split in any block that gets control with exec == 0 and has no restoring code in prologue?
I just did it by example of the code that already does same for the blocks with interference - a bit later below:

Now the correct diff uploaded

Oops. In fact the diff above is not that I was intended to upload. The SIInstrInfo::IsValidForLISplit is a complete nonsense. Probably deleted a part of the function by accident...
I'll get back and upload the working one.

diff rebased to latest trunk

No redundant branches anymore

The only difference is that now these redundant branch is inserted by MachineBasicBlock::updateTerminator() as Matt suggested.

changed as requested by reviewer

Added MachineDominatorTree and MachineLoopInfo update after redundant block removal.

In D86634#2240606, @arsenm wrote:

Should also add a few cases with other empty block situations, including with debug info.

Also should add an example where the original problem occurred

Changes as requested by reviewer.

This small piece was missed from the change.

diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index 5f1afdd7f10..7180e0a8d52 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -634,6 +634,9 @@ void SIInstrInfo::copyPhysReg(MachineBasicBlock &MBB,
   }

In D82194#2111508, @michel.daenzer wrote:

This change broke thousands of piglit gpu profile tests with Mesa radeonsi on Navi 14.

and as well as the failures it caused spurious debug output like:

Test case 'dEQP-VK.subgroups.arithmetic.framebuffer.subgroupmax_int_tess_eval'..
S_CMP_LG_U32 killed $sgpr2_sgpr3, 0, implicit-def $scc
S_CMP_LG_U32 killed $sgpr0_sgpr1, 0, implicit-def $scc
  Fail (Failed!)

In D82194#2111508, @michel.daenzer wrote:

This change broke thousands of piglit gpu profile tests with Mesa radeonsi on Navi 14.

udivrem.ll checks updated

Formatting fixed. test extract_vector_dynelt.ll changed.

Code changed according to the reviewer request

test corrected

Ping again. Could you please take a look?

Explicit call to isSafeToFoldImmIntoCopy added. Corresponding COPY converted to S_MOV right away.
2 test cases added to check for copy to subreg and inappropriate move opcodes.

In D80434#2051618, @rampitec wrote:

In D80434#2051603, @alex-t wrote:

In D80434#2051602, @rampitec wrote:

You do not even need a loop to get PHI. A PHI might occur after an simple IF.
Nevertheless, patch retains an illegal copy which it is supposed to remove. The fact that it works in this testcase does not mean it will always work.
I would suggest to fold the immediate copy instead. I.e. you can replace COPY with an S_MOV_B32 (given it fits 32 bit of course).

The following code does it in one of the next iterations

// If we are just copying an immediate, we can replace the copy with
// s_mov_b32.
if (isSafeToFoldImmIntoCopy(&MI, DefMI, TII, SMovOp, Imm)) {
  MI.getOperand(1).ChangeToImmediate(Imm);
  MI.addImplicitDefUseOperands(MF);
  MI.setDesc(TII->get(SMovOp));
  break;
}

What if not isSafeToFoldImmIntoCopy?

In D80434#2051602, @rampitec wrote:

You do not even need a loop to get PHI. A PHI might occur after an simple IF.
Nevertheless, patch retains an illegal copy which it is supposed to remove. The fact that it works in this testcase does not mean it will always work.
I would suggest to fold the immediate copy instead. I.e. you can replace COPY with an S_MOV_B32 (given it fits 32 bit of course).

Explicit check move source operand to be immediate.

In D80434#2051336, @rampitec wrote:

Every mov isMoveImmediate(), it is just a property on MCDesc.
I also assume it relies on the folding later. If that folding will not happen for whatever reason it will be a broken IR. Right?

ping

test added

test reworked.

Explicit register class comparison changed to constrain reg class. Test added.

LGTM

changed code passed through the clang-format

In D78091#2011048, @rampitec wrote:

Can you add tests with both immediates? You will probably need to -start-before=amdgpu-isel or -O0 if it will be constant folded.
Also fix formatting issues.

Selection specific test (carryout-selection.ll) added

a few more corrections

Both operands can be immediate - handled. Some other changes to follow up the discussion.

In D78091#1981388, @rampitec wrote:

You need to add tests for selection and moveToVALU, including immediates and wave32.

Src1 immediate case handled. Formatting improved.

static const

In D76230#1924449, @arsenm wrote:

Test name is misleading, this should just go in the realgar sext test. Also I would expect this to already be well covered

Ping!

Test updated

In D73815#1857100, @arsenm wrote:

In D73815#1857088, @alex-t wrote:

I agree that current expensive walk on IR should be removed.
I initially consider it as a temporary solution that allows to switch to the new concept
of assignment correct register classes keeping in mind to eventually find another way to workaround CF results exceptions.

What I conceptually don't like here is mixing the "divergence" and register class notions.
The mask produced by the CF intrinsics is always scalar - not same as always uniform.

Scalar is the only modeled concept of uniform we have. We don't currently try to model workgroup uniform, so these should be the same

It should work if we can guarantee that the values produced by the CF intrinsics are only used by the another CF intrinsics.
I mean something like AND/OR the uniform mask with some divergent value and then feeding result to another CF...
but normally this should not happen.
Once again: I like getting rid of requiresUniformRegister but insist on the extended testing.
Also, the trouble with LCSSA use of the uniform mask out of the divergent loop need to be solved anyway.

I agree that current expensive walk on IR should be removed.
I initially consider it as a temporary solution that allows to switch to the new concept
of assignment correct register classes keeping in mind to eventually find another way to workaround CF results exceptions.

llvm.amdgcn.update.dpp test updated

i64 tests added

In D73483#1849587, @arsenm wrote:

I expected this to fix the test XFAIL'd in c12f046eb96f8462b3fd3889344ba344de5ace1f, but it seems to to have?

I haven't seen any tests XFAIL'd by this c12f046eb96f8462b3fd3889344ba344de5ace1f commit.
What did you mean? Could you explain please?

Diff corrected. GFX10 test added.

As far as I remember the origin of this hack: