The tests you changed had 2 byte constants vs the 4 byte ones in the remaining f32 tests. But I don't know if that is significant.
LGTM

I've found a difference between the if statement and foreach statement which seems to be related to casting. See https://bugs.llvm.org/show_bug.cgi?id=50486

In D101474#2725954, @foad wrote:

That is what you suggested last time on https://reviews.llvm.org/D99376

OK :-)

So what caused the test failures that prompted the revert of D99376? Did you just need to update some more lit tests?

In D101474#2725051, @foad wrote:

This patch revises d35d8da7d6ac6c08578ec0569b072292631691e0.
It contains the commute opportunities excluding float insts

Maybe mention D99376 here rather than just the commit hash?

Why are you excluding float insts now? Is it related to the test failures that caused you to revert D99376, or is it a different problem?

That is what you suggested last time on https://reviews.llvm.org/D99376

LGTM

LGTM

LGTM

In D99376#2657778, @rampitec wrote:

In D99376#2656686, @Joe_Nash wrote:

Reverted since I don't fully understand the issue. It passed all tests on my system, but then upon rebasing top of tree it didn't. So I suspect the commutes may not be deterministic/ well constrained.

I have a bad suspicion here: that is not OK to just commute source modifiers for opsel. A DST_OP_SEL shares bits with $src0_modifiers, so it needs to be transferred to the new src0 modifiers and masked in src1. I suspect it does not happen.

Reverted since I don't fully understand the issue. It passed all tests on my system, but then upon rebasing top of tree it didn't. So I suspect the commutes may not be deterministic/ well constrained.

Confirmed that modifier operands are appropriately swapped if the
instruction is commuted, including opsel modifiers. Added at tests
for it

update test using add_lshl

remove sub as commutative and add add_lshl.
Its pretty bizarre to me that no testing cared if sub was commutative.

add test

It is unclear to me how to force a commute for testing purposes, but if anyone has an idea I can put that in. Vulkan short testlist passes fine with this patch. I will also test a compute psdb.

remove null initialization and fix clang-format

remove erroneous addition of OMod to cvt_flr_i32_f32 &
cvt_rpi_i32_f32

In D97689#2594419, @Joe_Nash wrote:

In D97689#2594395, @dp wrote:

I was surprised to discover that codegen tests include encodings. Is there any reason for that? Should I remove or update them?

Yes, it is surprising. I am inclined to say remove the encodings.

In D97689#2594395, @dp wrote:

I was surprised to discover that codegen tests include encodings. Is there any reason for that? Should I remove or update them?

I'm getting failing tests in check-llvm-mc-disassembler-amdgpu when applying this patch, can you take a look at that please?

LGTM

In D94341#2490208, @foad wrote:

Looks OK to me. Does this enable any immediate simplifications? Or is it just to help with future work?

Address format comments of foad.

Apply nfc edits suggested by dp

combined !con per comment by foad

changed B_encoding predicate to be 10_3Insts

remove inst from gfx1030 only, instead of all gfx10

whitespace change for lint and rerun build

remove accidental commented code

add OtherPredicates and blank line for rampitec

In D89738#2339932, @rampitec wrote:

Are there hazards associated with SOP, where we have it lowered to real instructions?

I'm not sure exactly what you mean, but there is a workaround for branch instructions, that operates on the real instructions. Original fix at https://github.com/llvm/llvm-project/commit/9ab812d4752b2a1442426db2ccc17dc95d12eb04

Also please run PSDB for this change.

Will do

Thank you for the review. I do not yet have commit access so please commit this on my behalf.

I do not have commit access, please commit the patch for me.