- User Since
- Feb 16 2017, 7:55 AM (301 w, 6 d)
Retitle and correct as suggested by Joe.
Fri, Nov 25
Thu, Nov 24
Correct as suggested by Jay.
Wed, Nov 23
Corrected as suggested by Joe.
Joe, thanks for your valuable comments!
LGTM. Nice improvements!
Tue, Nov 22
Fri, Nov 18
Thu, Nov 17
Updated as suggested by Joe:
Wed, Nov 16
! 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.
Tue, Nov 15
I suggest doing renaming in a separate patch because it is a separate problem introduced before this change.
Add a comment describing InstInfo::getRegIndices.
Mon, Nov 14
Why does it change the error position?
Fri, Nov 11
The currently reported position is the beginning of the first operand and in the current design there seems to be no way to do better
Could you add a test to check the reported error position? Unfortunately, AsmMatcher often fails to report the location of offending operand. Moving the check to validator may improve diagnostic by using getImmLoc.
Thu, Nov 10
That was my guess as well. But isn't it right that we still want to be able to disassemble TFE stores, even if functionally useless?
For some reason SP3 additionally accepts TFE forms with vN VData operands where v[N:N+1] is expected, so for example buffer_load_dword v1, v0, s[4:7], s1 glc tfe and buffer_load_dword v[1:2], v0, s[4:7], s1 glc tfe are both accepted and produce same opcodes. It should be possible to support that with a separate change, if needed.
AFAIK, tfe should be supported for loads and atomics only. I think that there is no sense to use tfe with stores.
SP3 is very forgiving and does not check many limitations.
Wed, Nov 9
Mon, Nov 7
Overall it is a nice idea, but I'm not sure how useful it would be for non AMDGPU backends. I suspect that most backends do not mix optional and mandatory operands (but I may be wrong).
Thu, Nov 3
Add tests with two literals.
Yes, it should be allowed.
Revert unnecessary (and incorrect) operand type changes as Joe suggested.
Wed, Nov 2
Tue, Nov 1
LGTM. Could you check that instructions without operands like v_nop are properly classified? Unfortunately disassembler does not print _e64 for such instructions, so they look like native VOP3 opcodes.
Oct 28 2022
Oct 27 2022
Oct 25 2022
The patch may be useful, but I’m concerned with file naming. I think the naming should be consistent. Right now we have vop3, vop3c, vop3cx and now vop3_from…. Maybe we should think over test naming?
LGTM (with or without MC code corrections).
Oct 24 2022
Overall looks fine, but I want somebody to look at codegen changes. You could leave MC code refactoring to me (but I'm not sure if I'll be able to improve code that much).
Oct 21 2022
Update as suggested by Joe.
Did you observe any instructions that assembled when they shouldn't? It looks like only differences in error message from the tests.
Oct 20 2022
Oct 19 2022
Oct 18 2022
Yes, the issue only manifests itself with modifiers applied to inline constants.
Oct 14 2022
Oct 13 2022
Oct 11 2022
Oct 10 2022
Oct 7 2022
MC changes LGTM.