User Details
- User Since
- Jan 28 2021, 4:00 AM (80 w, 2 d)
May 18 2022
LGTM
May 1 2022
It's taken me a very long time to find time to read though this patch (I'm very sorry about that). The approach seems nice and straight forward & the generated code looks good. Thanks a lot for finding the time to get this through. 😅
Feb 15 2022
LGTM. Might be worth waiting for @myhsu to review, but I don't think there's anything controversial. Thanks! 😄
Thanks for this, I'm really enjoying the new syntax, it's so much easier to follow. I'm a little bit unsure about the test change.
LGTM. I had underestimated how complicated it would be to generate relocations, but this looks good.
Feb 14 2022
This LGTM. I can't spot any code style issues and it should only affect the 68k for now. I like how it looks in the tablegen files. Nice work. :)
Jan 15 2022
Style is fine, tests run clean for me.
Dec 14 2021
LGTM
Dec 11 2021
LGTM. There's one comment that I think might be wrong. This is cool to see and I favour it much more than the existing code beads, IMO it's much more readable. Thanks! :)
Nov 9 2021
I've been meaning to take a look since it shouldn't need much, but I've not gotten around to it yet.
Oct 25 2021
I'm still getting test failures with this updated patch.
Oct 21 2021
@AnnikaCodes, I'm seeing test failures from this patch. It looks like it's generating SETCC nodes which don't return i8 (which the M68k requires).
I'm happy to. Are you ready for this to land @AnnikaCodes?
Sep 29 2021
Sep 17 2021
Aug 27 2021
Aug 26 2021
Fair enough, makes good sense. I was trying to chip away at things which didn't involve messing with the instruction definitions (since they feel a little up in the air still).
Aug 25 2021
Make the commit title more precise.
Update the commit message.
Change over to using KindTy as the name of the enumeration.
Aug 24 2021
Aug 23 2021
May 2 2021
LGTM
Somehow my comment got trimmed (I think because a new diff appeared between me writing it and pressing submit).
One nitpick, I don't believe it needs addressing but wanted to point it out anyway.
The relocation tests look a little messier than the others (were they generated from the old tests?) but it's pretty clear and more representative of what LLVM will output I guess.
Apr 19 2021
Fixup one missed instance of the "don't use braces" rule.
Fixed up code style.
Apr 17 2021
Fix up the other switch statement that I somehow missed.
Shorten switch to a single assert.
Apply review feedback.
Apr 14 2021
Add loop changes missed from previous diff.
I think this is now ready for review proper.
Improve code style, add encoding tests to instructions.s.
Apr 13 2021
Apr 12 2021
@myhsu when you get a moment, can you check that this is all OK now? :)
Apr 6 2021
Right, that makes sense!
An argument could be made that the parseIdentifier routine could be modified or overridden wherever appropriate and given custom logic.
The benefit being that target AsmParsers have the opportunity to parse the starting symbol as something else. The drawback being that stitching everything back together is more complicated (looking at the code, I'm not convinced that it'd parse $$ as an identifier, for example).
Use assembly comments in the assembly test rather than C++ comments
- Hide private implementation details of M68kAsmParser
- Remove superfluous FileCheck parameter from test
- Add migration notice to test
Apr 5 2021
I'm new to reviewing code here, so call me out if I mess up the etiquette. :)
Apr 4 2021
Rebase onto changed AsmLexer patch
Apr 2 2021
Pulled the use of UseMotorolaIntegers back into this patch.
Rebased, fixed some bugs & added some tests.
Remove header to appease clang-tidy
Apply review feedback, add some tests and a flag for llvm-mc.
I've added a test (it uses many of the instructions which are defined with both Motorola & MIT operand syntax) and tidied everything up.
Rebase onto latest main
Reworked parsing and added the first test
Mar 31 2021
Mar 19 2021
Made as_imm16 comment more generic
Mar 18 2021
Mar 17 2021
I've marked this 'Ready for Review' since otherwise notifications don't happen. To be clear though, this is not ready to merge.
I've marked this 'Ready for Review' since otherwise notifications don't happen. To be clear though, this is not ready to merge.
Fix one superfluous DReg: the rr arithmetic includes the DA bit for the operand already.
Remove operand type changes. (These will appear in another differential shortly.)
Yep, converted those back into sequences. Thanks.
Change explicit register lists back into sequences where possible.
Mar 16 2021
Use Register second parameter instead of an extra 'let'
I don't have commit access, so if someone could land this for me, I'd appreciate it! :)