Add support for DONE, RETRY, SAVED, and RESTORED (v9 §A.11 & §A.47).
Those instructions are used for low-level interrupt handling and register window management by OS kernels.
Details
Diff Detail
Event Timeline
llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp | ||
---|---|---|
687 | Shouldn't this be impossible due to the predicate, and thus be an assert? | |
694 | isUInt<32>(ImmValue)? | |
700 | Use MCInstBuilder throughout? Avoids all the { ... } below, too... | |
1134 | Use a TableGen GenericTable for this to avoid having to define the forward mapping here and the backward mapping later? (See RISCVSystemOperands.td for an example). |
llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp | ||
---|---|---|
687 | Probably just me misunderstanding it but during testing I found out that even with 32-bit -mtriple=sparc it still recognizes SETX anyway, that's why I'm putting the condition as a normal error instead of an assert. |
Implement review suggestions:
- Use isUInt<32>(ImmValue) instead of manually checking ranges
- Use MCInstBuilder to expand setx pseudoinstruction
- Create a table to hold ASI symbolic names
llvm/lib/Target/Sparc/SparcInstrInfo.td | ||
---|---|---|
1834–1838 | I'm confused by what this comment means, as you're defining a new instruction. Is the intent to define an assembler alias only? |
llvm/lib/Target/Sparc/SparcInstrInfo.td | ||
---|---|---|
1834–1838 | Yeah, my intent is to define a rdpr %fq, $rd alias, but the encoding of %fq is different between v8 and v9 versions of the ISA, so I had to do that to convince the assembler to encode it properly when emitting code for v9. |
It looks like there are several independent changes. A few smaller diffs would be much easier to review.
llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp | ||
---|---|---|
454 | It should have its own Kind and union member. Not sure if tablegen provides you with an enum, but if it does, the enum could be used. | |
1123 | Why not just getTok().is(AsmToken::Integer) and getTok().getIntVal() on success? | |
1131 | Is space after '#' allowed? If not, getLexer().peekToken(false) should be used. | |
1150 | The location is not precise. It points before the last whitespace (there may be several). | |
1234 | This should return MatchOperand_ParseFail. NoMatch implies no tokens were consumed. | |
1249 | Missing return? | |
1255 | Why is LParen here? | |
1621–1684 | Do you know why Sparc does not use autogenerated MatchRegisterName? (ShouldEmitMatchRegisterName = 0 in Sparc.td.) | |
llvm/lib/Target/Sparc/SparcASITags.td | ||
17–22 | It looks like all tags have an alternative name, just pass it as template parameter. | |
llvm/lib/Target/Sparc/SparcInstrAliases.td | ||
513 | It should be simm13Op, though it does not currently matter that much as it is missing AsmOperandClass, so no overflow checking is done. |
Any advice on how to split it? This is basically just a bunch of instruction definitions, the complication mostly arises since some of the parsing needs to be handled on the C++ side.
llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp | ||
---|---|---|
454 | Not entirely sure if I can/how to do this, any example code I can refer to? | |
1123 | Same reason with the LParen matcher, this is to handle constant expressions for ASI identifier. | |
1249 | Should be no problem since it'll fall through the if check below it, but will anyway for clarity, thanks. | |
1255 | Because using a constant expression for ASI identifier value (for example ldxa [%o0] (0x80 + 0x08), %o1) is also legal. | |
1621–1684 | Unfortunately no. I am still new to the backend so for these parts of the code I'm just following what the original code already does. |
llvm/test/MC/Sparc/sparcv9-atomic-instructions.s | ||
---|---|---|
18 | The indentation seems unnecessary. It seems that we can unindent all instructions. That's the more common style for instruction encoding in other targets' testing as well. |
llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp | ||
---|---|---|
454 | Roughly:
| |
1123 | I'd personally prefer parseExpression() instead of parseSparcAsmOperand() if the first token is not '#'. Up to you. | |
1249 | Actually there was a problem because parseSparcAsmOperand consumes tokens :) | |
1255 |
That's weird. Can you add a couple of testcases using this syntax?
I think not. |
- Implement formatting suggestions for new testfile
- Add constant expression forms of tests
- Fix return value for CAS operands parsing
- Add separate enum kind for ASI tags
llvm/test/MC/Sparc/sparcv9-atomic-instructions.s | ||
---|---|---|
18 | I can do that with new testfiles, but for old ones I feel like it's better to follow old formattings? |
Looks fine to me. It would still be better to create separate revisions for independent changes.
I still think the best way forward is to split this set of changes into several. It would be much easier to review, ensure test coverage is sufficient and commit messages would be more helpful.
I can see several independent changes here (I may have missed something):
- Adding DONE/RETRY/SAVED/RESTORED instructions
- Adding RDFQ variant
- Adding CAS variants
- Adding SETX pseudo instruction
- Adding reg+imm mode to some memory instructions
- Adding new mnemonics to control registers
- Supporting ASI tags
- Other minor changes
I think the code looks fine but I know nothing about SPARC. Don't really care about splitting this up or not, though I generally prefer minimal patches
It should have its own Kind and union member. Not sure if tablegen provides you with an enum, but if it does, the enum could be used.
This is "safer" and allows you to avoid converting to MCExpr and back.