No objections from MIOpen side.

In D139455#3996367, @arsenm wrote:

We have preserve-sign behavior and ignore positive-zero mode entirely. Also the modifiers are silently ignored for ieee mode =0. I don't remember what the wrong behavior depending on the denormal mode was, beyond don't use these without flushing

Just in case, am I correct that the compiler engages output modifiers only when "denormal-fp-math"="positive-zero"? AFAIU this is necessary because output modifiers force -0 to +0.

In D129570#3717972, @pfultz2 wrote:

I could extend it to support everything except explicit casts if you think that would be better.

LGTM

A typo in the doc, but otherwise LGTM.

Make it so!

Good!

lol, you've already made null reads free from the constant bus POV ;)
LGTM

In D65229#1652664, @artem.tamazov wrote:

Interesting question. Does null SGPR reads occupy constant bus?

LGTM

Interesting question. Does null SGPR reads occupy constant bus?

This looks good!

LGTM

LGTM

LGTM!

LGTM!

Please make this small fix and go ahead.

LGTM

Very good, thanks!

Great, thanks!

What would occur if offset exceeds int16 due to destination label is too far?

Looks good.

LGTM. Recommendation: it would be nice if error message will be more descriptive (where is does not require too much work)

LGTM

Looks good.

LGTM

None objections.

LGTM

In D59305#1428852, @dp wrote:

Shader programming documents state that MUBUF SOFFSET is "SGPR to supply unsigned byte offset. Must be an SGPR, M0 or inline constant."
Also note that our tests are based on SP3 tables which also allow inline constants.
Of course, using fp inline constants in this context is meaningless.

LGTM

Looks good from assember's side.

Good, thanks!

LGTM

This patch LGTM

Do we support different constants if one is literal and another one is inline constant?

s_and_b32 s2, 2, 1234

AFAIR gpu supports arbitrary number of inline constants.

Perhaps I am not the right person to approve this, but LGTM provided that this enables CO v2/v3 switching.
Would it work with "clang -x assembler..." as well?

Good job, thanks!

LGTM

Perhaps fixes needed.

LGTM, thanks.

In D47736#1140852, @kzhuravl wrote:

In D47736#1140847, @artem.tamazov wrote:

Is it so that the updated documentation is valid only if code-object-v3 is specified?

Yes

Is the V3 code object compatible with the latest ROCm release?

Not yet

Is it so that the updated documentation is valid only if code-object-v3 is specified?
Is the V3 code object compatible with the latest ROCm release?

LGTM

LGTM

Adding Dmitry as he is more fluent in this domain.

LGTM.

LGTM

Is it so that max_scratch_backing_memory_byte_size is not needed at all for Gfx7-9?

LGTM

Looks good.

Great, thanks.

Looks good.

LGTM

In D45250#1056997, @dp wrote:

Corrected asm tests to use traditional notation (GFX89 instead of VI)

LGTM

Looks good.

LGTM.

Looks good.

Looks good.

LGTM

LGTM

Looks good.

LGTM

LGTM

In D44779#1046500, @dp wrote:

But maybe it would be safer to support this opcode for gfx8/9 only to mimic SP3 assembler?

In D44685#1043335, @timcorringham wrote:

This implements Bug 36347

Does this resolve any bugzilla?

Adding Dmitry.

LGTM

LGTM

It seems that -mattr corrupts the design. What is the purpose of this option?

Dmitry, I believe you can go ahead.

LGTM

In D43950#1024132, @dp wrote:

Regarding TFE, documentation says that TFE is valid for "fetch" instructions only. By design, TFE is useless with stores.
I believe this instruction is not a "fetch".

The change requires LDS bit in BUFFER_STORE_LDS_DWORD. This looks incorrect. LDS bit affects only MUBUF reads.

LGTM

In D43472#1013187, @dp wrote:

In D43472#1013066, @artem.tamazov wrote:

LGTM. I recommend adding an erratic case with both "lds" and "tfe" to the tests.

Added...

LGTM. I recommend adding an erratic case with both "lds" and "tfe" to the tests.

Looks good except come comments.

LGTM.
BTW it would be nice to have a comment explaining the limitations of MIMG support. In short:

• There is a fundamental limitation: only index of the beginning VGPR of address and data is specified/encoded. The number of actually used VGPRs are set implicitly (in the T#) and thus not known during assembling and disassembling.
• This limitation can not be overcame unless violation of the SP3 compatibility requirement
• Also there is a drawback related to CodeGen: the number of used VGPRs can be set to the power of 2 only, but actual number of VGPRs used is more diverse.

In D41617#977175, @rampitec wrote:

I believe the source of inconsistency is the original definition of ID_SYMBOLIC_LAST_, which does not point to a last value in the enum, but a last + 1. This can be changed in a separate patch.

In D42186#980293, @dp wrote:

This is an interesting idea, but T# is assumed to be in registers. Also note that GFX10 added a 'dim' field to encode address size.

Dmitry, please consider the following (excerpt from GCN ISA manual):

LGTM

Let's reach consensus.

If possible, please add some negative test(s), i.e. SDWA with non-inlineable constant(s). See also style-related recommendation. Otherwise fine.

Some style-related fixes are desirable, otherwise LGTM.

Please also add some testcases to test\MC\AMDGPU\sopk.s and, possibly, to sopk-err.s.