This is an archive of the discontinued LLVM Phabricator instance.

[X86AsmParser] Refactoring, (almost) NFC.
ClosedPublic

Authored by coby on Aug 16 2017, 7:10 AM.

Details

Summary

Some refactoring to X86AsmParser, mostly regarding the way rewrites are conducted.
Mainly, we try to concentrate all the rewrite effort under one hood, so it'll hopefully be less of a mess and easier to maintain and understand.
naturally, some frontend tests were affected: D36794

Diff Detail

Repository
rL LLVM

Event Timeline

coby created this revision.Aug 16 2017, 7:10 AM
coby edited the summary of this revision. (Show Details)Aug 16 2017, 7:15 AM
avt77 added a comment.Aug 18 2017, 3:36 AM

IntelExpr::isValid looks rather strange because it validates possible multiplication only. But if we have Scale == 1 and don't have either BaseReg or Disp?

rnk edited edge metadata.Aug 18 2017, 5:24 PM

I didn't get to this this week, but this code could definitely use some refactoring. :)

coby added a comment.Aug 19 2017, 3:08 PM

IntelExpr::isValid looks rather strange because it validates possible multiplication only. But if we have Scale == 1 and don't have either BaseReg or Disp?

first of all, thanks for your comment.
AOK_IntelExpr is a rewrite mechanism, and as such its mandate is to be a dumb (but convenient) conveyor. 'isValid()', under those guidelines, checks some most trivial parts which can be inferred with ease. others parts are assumed to be validated on the path that led to its emission (either pre or post).
Specific to the scenario you've described - if we 'have' Scale == 1 (which is the default case) any other part is 'considered' legal (to the purpose of this rewrite, again - this is simply a conveyor - we expect validation effort to be conducted by the parser). Index/Base/ImmDisp will be emitted if present.

coby updated this revision to Diff 111880.Aug 20 2017, 6:14 AM

Removed AOK_Delete as it has no use anymore

mcrosier resigned from this revision.Aug 22 2017, 7:28 AM
rnk added inline comments.Aug 23 2017, 1:29 PM
lib/Target/X86/AsmParser/X86AsmParser.cpp
339–377 ↗(On Diff #111880)

Can you clang-format this?

604 ↗(On Diff #111880)

formatting

606 ↗(On Diff #111880)

I think HasSymbol would be a better name for this, and Sym != nullptr is a more readable way to force a boolean conversion.

1708 ↗(On Diff #111880)

I think ParseIntelMemoryOperandSize would be a better name for this.

1790 ↗(On Diff #111880)

Use != instead of <>, it's not commonly used.

coby updated this revision to Diff 112459.Aug 23 2017, 2:53 PM
coby marked 5 inline comments as done.

Addressed @rnk 's comments (thanks!)

rnk accepted this revision.Aug 23 2017, 5:14 PM

lgtm

lib/Target/X86/AsmParser/X86AsmParser.cpp
1306 ↗(On Diff #112459)

Woohoo, excited to see this go. :)

This revision is now accepted and ready to land.Aug 23 2017, 5:14 PM
coby added inline comments.Aug 24 2017, 1:25 AM
lib/Target/X86/AsmParser/X86AsmParser.cpp
1306 ↗(On Diff #112459)

Aye, won't yearn for it either !

This revision was automatically updated to reflect the committed changes.
llvm/trunk/lib/Target/X86/AsmParser/X86AsmParser.cpp