LGTM.

LGTM.

LGTM.

LGTM

LGTM, thanks!

In D149254#4298967, @foad wrote:

In D149254#4298864, @dp wrote:

IMO this change should be limited to GFX11 to preserve compatibility with existing assembly code.

Why GFX11? Do you mean because it was the first architecture introduced *after* you fixed the disassembler to output bound_ctrl:1 instead of bound_ctrl:0 in D97048?

IMO this change should be limited to GFX11 to preserve compatibility with existing assembly code.
Documentation should be corrected as well - see https://llvm.org/docs/AMDGPUModifierSyntax.html#bound-ctrl

LGTM. Nice refactoring!

LGTM

LGTM

LGTM.

LGTM.

LGTM, but I suggest moving the test to some other place. *_unsupported.s are used solely to test identification of unsupported opcodes. gfx7_err_pos.s may be a better place for this test.

Is there a ticket to be mentioned in the commit message?

There is no ticket. I found this bug while analyzing your patch and trying to break the parser.

Does your patch fix this bug?

I think that worked for CI only

The patch itself looks fine, but it breaks backward compatibility with existing code.
I think if you make offset mandatory for GFX6 and GFX7, the same should be done for other GPUs. But IMO preserving compatibility with existing code is more important.
BTW, have you noticed that SP3 does not allow 32-bit offsets with SMRD? LLVM assembler accepts these, but I do not know if HW can handle it.

LGTM.

LGTM.

In general, I like your idea and I see that further improvements are possible. Your second patch seems to better justify this approach.

LGTM

LGTM, thanks!

Very nice refactoring!

LGTM with a nit.

Overall looks good.

In most cases if instruction can't use some bits in encoding of an operand, those bits are set to default value in td file.

AFAIK, we use ? as a default value to allow disassembler ignore these bits.

LGTM.

By design, disassembler should be able to decode invalid code when possible. It is much more helpful for the user to see something like this:

LGTM.

Nice refactoring!

Update comment describing isSISrcOperand.

Rebase; update tests to add error line numbers.

Retitle and correct as suggested by Joe.

Ping.

Correct as suggested by Jay.

Corrected as suggested by Joe.
Joe, thanks for your valuable comments!

LGTM. Nice improvements!

Updated as suggested by Joe:

! In D137952, @Joe_Nash wrote:
Now I see that it does not iterate over MCInst operands. So clearly it is confusing :).
SrcIdx is an index into parsed src operands which is a subset of parsed operands.
So maybe these renames are most accurate:

getParsedSrcIndex -> getParsedOpIdxFromSrcIdx
SrcIdx -> CombinedSrcIdx
getParsedDstIndex -> getParsedOpIdxOfDst

In getRegIndicies
I think SrcIdx is acutally ComponentSrcIdx
Renames:
SrcIdx -> ComponentSrcIdx
getSrcIndex -> getCombinedMCSrcIdxFromComponentSrcIdx

obviously this is getting really verbose, but that's probably better than confusingly named.

Rebase.

I suggest doing renaming in a separate patch because it is a separate problem introduced before this change.

Add a comment describing InstInfo::getRegIndices.

Why does it change the error position?

Still LGTM.

LGTM.