Page MenuHomePhabricator

[RISCV][MC] Add support for evaluating constant symbols as immediates
ClosedPublic

Authored by asb on Sep 20 2018, 5:29 AM.

Details

Summary

This further improves compatibility with GNU as, allowing input such as the following to be assembled:

.equ CONST, 0x123456
li a0, CONST
addi a0, a0, %lo(CONST)

.equ CONST, 1
slli a0, a0, CONST

Note that we don't have perfect compatibility with gas, as it will avoid emitting a relocation in this case:

addi a0, a0, %lo(CONST2)
.equ CONST2, 0x123456

Diff Detail

Repository
rL LLVM

Event Timeline

asb created this revision.Sep 20 2018, 5:29 AM

Hi Alex, another solution could be calling parseExpression for AsmToken::Identifier in RISCVAsmParser::parseImmediate instead of calling parseIdentifier.

case AsmToken::String:
case AsmToken::Identifier: {
  if (getParser().parseExpression(Res))
    return MatchOperand_ParseFail;
  break;
}

parseExpression will call parsePrimaryExpr which contain the logical to call parseIdentifier and also try to get the symbol value when the symbol is variable.
What do you think?

asb added a comment.Oct 4 2018, 2:26 AM

Thanks Shiva, that seems like a better approach. It also allows for parsing CONST+1 etc. This needs more test coverage though. From an initial look, it seems lui a0, CONST+1 has a problem when using parsePrimaryExpr that needs investigating further.

asb updated this revision to Diff 180051.Jan 3 2019, 6:24 AM
asb edited the summary of this revision. (Show Details)

As suggested my Shiva, this patch has been updated to allow AsmParser::parseExpression to do the work when parsing an identifier.

I mentioned previously that I encountered an issue when trying to switch to this approach, but I'm not longer able to reproduce (so it was probably a mistake on my part).

Note that constant symbols can't be used with CSR operations. That would require changes toparseCSRSystemRegister. Same for parseBareSymbol (used in e.g. the call pseudoinstruction). Given that this patch is an incremental improvement that makes sense on its own, I'd prefer to follow up with separate patches for the other cases.

Does this look good to you?

Hi Alex,
Thanks for the updates, looks good to me.

shiva0217 accepted this revision.Jan 3 2019, 9:52 PM
This revision is now accepted and ready to land.Jan 3 2019, 9:52 PM
This revision was automatically updated to reflect the committed changes.