- User Since
- Feb 16 2017, 7:55 AM (355 w, 2 d)
Jun 4 2023
May 27 2023
May 16 2023
May 8 2023
Apr 26 2023
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
Mar 30 2023
LGTM. Nice refactoring!
Mar 29 2023
Mar 24 2023
Mar 23 2023
Feb 9 2023
Feb 8 2023
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.
Jan 24 2023
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.
Jan 13 2023
Jan 12 2023
Jan 10 2023
In general, I like your idea and I see that further improvements are possible. Your second patch seems to better justify this approach.
Jan 9 2023
Dec 30 2022
Dec 21 2022
Dec 20 2022
Dec 19 2022
Dec 16 2022
Dec 15 2022
Very nice refactoring!
Dec 13 2022
Dec 9 2022
LGTM with a nit.
Overall looks good.
Dec 7 2022
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.
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:
Dec 6 2022
Dec 5 2022
Dec 2 2022
Dec 1 2022
Update comment describing isSISrcOperand.
Rebase; update tests to add error line numbers.
Nov 30 2022
Nov 29 2022
Retitle and correct as suggested by Joe.
Nov 25 2022
Nov 24 2022
Correct as suggested by Jay.
Nov 23 2022
Corrected as suggested by Joe.
Joe, thanks for your valuable comments!
LGTM. Nice improvements!
Nov 22 2022
Nov 18 2022
Nov 17 2022
Updated as suggested by Joe:
Nov 16 2022
! 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
I think SrcIdx is acutally ComponentSrcIdx
SrcIdx -> ComponentSrcIdx
getSrcIndex -> getCombinedMCSrcIdxFromComponentSrcIdx
obviously this is getting really verbose, but that's probably better than confusingly named.
Nov 15 2022
I suggest doing renaming in a separate patch because it is a separate problem introduced before this change.
Add a comment describing InstInfo::getRegIndices.
Nov 14 2022
Why does it change the error position?