This is an archive of the discontinued LLVM Phabricator instance.

[mips] Add assembler support for .set push/pop directive.
ClosedPublic

Authored by tomatabacu on Aug 7 2014, 2:40 AM.

Details

Summary

These directives are used to save the current assembler options (in the case of ".set push") and restore the previously saved options (in the case of ".set pop").

Contains work done by Matheus Almeida.

Diff Detail

Event Timeline

tomatabacu updated this revision to Diff 12273.Aug 7 2014, 2:40 AM
tomatabacu retitled this revision from to [mips] Add assembler support for .set push/pop directive..
tomatabacu updated this object.
tomatabacu edited the test plan for this revision. (Show Details)
tomatabacu added a reviewer: dsanders.
tomatabacu updated this revision to Diff 12619.Aug 18 2014, 8:07 AM

Improved a couple of error messages.

tomatabacu updated this revision to Diff 12620.Aug 18 2014, 8:36 AM

Changed the CurrentFeatures and InitialFeatures data members of MipsAssemblerOptions from unsigned type to uint64_t (because of r215887).

dsanders added inline comments.Aug 19 2014, 8:20 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
54–56

Nit: variables should start with a capital

84

I've just realized the reason we shouldn't use a static member. A static member prevents multiple instances of llvm being used in the same application. For example, we wouldn't be able to assemble two different files with two different llvm instances in the same process.

I think we need to switch to the other implementation we discussed which is to replace the static InitialFeatures with an extra MipsAssemblerOptions object on the stack.

98

We should use unique_ptr here.

315–319

Please pass the current features as an argument to the constructor so we can't forget to call setCurrentFeatures()

test/MC/Mips/set-push-pop-directives.s
9

We should test the default values too

tomatabacu updated this revision to Diff 12753.Aug 21 2014, 3:47 AM

Addressed comments.
Improved the set-push-pop-directives.s test.

tomatabacu updated this revision to Diff 12754.Aug 21 2014, 3:51 AM

Fixed a minor formatting issue.

tomatabacu updated this revision to Diff 12771.Aug 21 2014, 7:11 AM

Replaced the use of std::stack with SmallVector to make it easier to implement the .set mips0 directive (http://reviews.llvm.org/D4957).

dsanders edited edge metadata.Sep 4 2014, 7:52 AM

LGTM with a few nits.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
46

The argument should be const.

305

Nit: The aim isn't to cache the options. The aim is to treat all levels of the stack uniformly. The only special bits about the bottom value is that the user can't change it (because they can't pop that far), and that we will overwrite the top stack element with the values from the bottom when '.set mips0' is used.

306–307

Nit: Please declare and assign in the same statement

2684–2685

Always keep an element on the stack.

Two elements.

This is how we cache the initial options.

As mentioned above, caching isn't the purpose of the outermost environment.

2702–2703

Nit: Please declare and assign in the same statement.

2705

Nit: This isn't necessary. It's about to go out of scope.

tomatabacu updated this revision to Diff 13307.Sep 5 2014, 3:38 AM
tomatabacu edited edge metadata.

Addressed reviewer's concerns.
Further clarified comments.
Rebased.

dsanders accepted this revision.Sep 5 2014, 8:34 AM
dsanders edited edge metadata.

LGTM, Thanks.

This revision is now accepted and ready to land.Sep 5 2014, 8:34 AM
tomatabacu closed this revision.Sep 9 2014, 3:25 AM