Page MenuHomePhabricator

[ARM64] Re-work parsing of ADD/SUB shifted immediate operands
ClosedPublic

Authored by bsmith on May 8 2014, 4:01 AM.

Details

Summary

The parsing of ADD/SUB shifted immediates needs to be done explicitly so that better diagnostics can be emitted, as a side effect this also removes some of the hacks in the current method of handling this operand type.

Additionally remove manual CMP aliasing to ADD/SUB and use InstAlias instead.

There is no testcase for this fix since it is part of porting over MC/AArch64/basic-a64-diagnostics.s, which will be enabled for ARM64 once everything is fixed in it.

Diff Detail

Event Timeline

bsmith updated this revision to Diff 9206.May 8 2014, 4:01 AM
bsmith retitled this revision from to [ARM64] Re-work parsing of ADD/SUB shifted immediate operands.
bsmith updated this object.
bsmith edited the test plan for this revision. (Show Details)
bsmith added a reviewer: t.p.northover.
bsmith set the repository for this revision to rL LLVM.
bsmith added a subscriber: Unknown Object (MLST).
t.p.northover edited edge metadata.May 8 2014, 5:03 AM

Hi Bradley,

I've got a few more comments on this one...

lib/Target/ARM64/AsmParser/ARM64AsmParser.cpp
581–583

I don't think these are correct here. They're for unpacking a binary encoded shift, but tryParseAddSubImm gives no indication that's what it's creating (& the name ShiftAmount would be misleading if it did).

2309–2310

Given its name and current purpose, this function seems more generic than necessary. (E.g. accepting arbitrary shift amounts when only 12 is ever valid).

Do you intend to use it for other cases, or should it be simplified?

bsmith added inline comments.May 8 2014, 5:11 AM
lib/Target/ARM64/AsmParser/ARM64AsmParser.cpp
581–583

Good catch, this is wrong since it's just the amount not the special encoded amount. I just so happened to get away with this since LSL = 0..

2309–2310

No it was only ever intended for this purpose. The 0/12 only logic is in the predicate function, but if you think it makes more sense to have it here instead then I can change this.

bsmith updated this revision to Diff 9219.May 8 2014, 7:39 AM
bsmith edited edge metadata.

New patch that removes the incorrect use of ARM64_AM shift encodings in isAddSubImm and tightens up the shift parsing in tryParseAddSubImm to disallow garbage shifts. As for simplifying tryParseAddSubImm, I'm not convinced that only allowing 12/0 there is going to actually simplify anything, but rather just move code from isAddSubImm.

bsmith updated this revision to Diff 9222.May 8 2014, 7:42 AM

(Minor fix to previous patch)

Hi Bradley,

Fair enough. The difference is marginal either way on that one. I did spot another nit-pick though:

lib/Target/ARM64/AsmParser/ARM64AsmParser.cpp
588

I think this might be undefined behaviour if we're actually in the isImm case: accessing a union via a member that wasn't most recently written, or whatever.

If it was me I'd handle isShiftedImm entirely within its if block and reserve the rest of the function for the non-shifted case.

bsmith updated this revision to Diff 9224.May 8 2014, 8:11 AM

Fixup potential union related undefined behavior in isAddSubImm. Unfortunately this can't reasonably be done quite as suggested without code duplication since the classifySymbolRef section needs to work for both the Imm and ShiftedImm cases.

Hi Bradley,

I think this looks good now. Thanks for going through so many revisions!

Tim.

bsmith accepted this revision.May 8 2014, 8:49 AM
bsmith added a reviewer: bsmith.
This revision is now accepted and ready to land.May 8 2014, 8:49 AM
bsmith closed this revision.May 8 2014, 8:49 AM