Page MenuHomePhabricator

[X86] Fix incomplete handling of register-assigned variables in parsing.
ClosedPublic

Authored by niravd on Thu, Jan 3, 12:10 PM.

Details

Summary

Teach operand parsing to distinguish between assembler variable assigned to
named registers and those assigned to immediate values.

Diff Detail

Repository
rL LLVM

Event Timeline

niravd created this revision.Thu, Jan 3, 12:10 PM
niravd updated this revision to Diff 180135.Thu, Jan 3, 1:52 PM

Rebase after unifying (%dx) fixup hack in parseinstruction.

Could use some test-cases for the error parsees, not just the successful parses.

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
1950 ↗(On Diff #180135)

Since this is quite long, it may make sense to put the whole thing into ParseMemOperand -- perhaps renaming 'ParseX86MemOperandOrRegister'.

1971 ↗(On Diff #180135)

I think it'd be clearer to create a temporary name e.g. "Expr" to use instead of "Imm" inside this block, for the expression value which is either an imm or register. And only assign to Imm in the "else" of the dyn_cast<X86MCExpr> conditional.

1975 ↗(On Diff #180135)

Typo -> "Parse out"

2020 ↗(On Diff #180135)

This entire block should be split into a separate "isAtMemOperand" helper function.

2023 ↗(On Diff #180135)

I'd write > 0 here, the >= 1 vs the > 1 below confused me for a second.

2030 ↗(On Diff #180135)

These lower cases are effectively implementing a "peekIdentifier" operation. It would be good to have a comment to that effect. Or, maybe peekIdentifier should even be split out as a separate function.

2061 ↗(On Diff #180135)

Is this guaranteed to be true?

2070–2071 ↗(On Diff #180135)

"If we're not at a "(" this is an immediate expression." is confusing, because it sounds like "this" is the upcoming tokens, not those already parsed. Probably can just be removed as redundant witht he first sentence.

2090 ↗(On Diff #180135)

No error message if !isa<X86MCExpr>?

niravd updated this revision to Diff 180702.Tue, Jan 8, 11:17 AM
niravd marked 11 inline comments as done.

Add error tests and do minor reorganization to parseExpression to existant extra invalid token errors.

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
2061 ↗(On Diff #180135)

Yes. The previous peekidentifier should return true if it was a register.

jyknight accepted this revision.Thu, Jan 10, 6:31 PM
This revision is now accepted and ready to land.Thu, Jan 10, 6:31 PM
This revision was automatically updated to reflect the committed changes.