This is an archive of the discontinued LLVM Phabricator instance.

[mips] Add assembler support for '.set mipsX'.
ClosedPublic

Authored by tomatabacu on Jun 12 2014, 7:55 AM.

Details

Summary

This patch also fixes an issue with the way the Mips assembler enables/disables architecture
features. Before this patch, the assembler never disabled feature bits. For example,
.set mips64
.set mips32r2

would result in the 'OR' of mips64 with mips32r2 feature bits which isn't right.
Unfortunately this isn't trivial to fix because there's not an easy way to clear
feature bits as the algorithm in MCSubtargetInfo (ToggleFeature) only clears the bits
that imply the feature being cleared and not the implied bits by the feature (there's a
better explanation to the code I added).

Diff Detail

Event Timeline

matheusalmeida retitled this revision from to [mips] Add assembler support for '.set : 1) 'mips1' 2) 'mips2' 3) 'mips3' 4) 'mips4' 5) 'mips5' 6) 'mips32' 7) 'mips32r6' 8) 'mips64r6'.
matheusalmeida updated this object.
matheusalmeida edited the test plan for this revision. (Show Details)
matheusalmeida added reviewers: dsanders, vmedic.
dsanders edited edge metadata.Jun 18 2014, 3:15 AM

The patch looks good to me but as you say, it needs to be reviewed on-list because of the change to MCSubtargetInfo

test/MC/Mips/set-mips-directives-bad.s
5–7

I misread this the first time since ll is both an instruction and a filetype. Could you add write 'the ll instruction' or similar?

matheusalmeida edited edge metadata.
vmedic edited edge metadata.Jun 18 2014, 5:34 AM

Looks good to me with a nit(comment). I agree it should be sent to commit list for a review.

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

I think this comment is not correct, shouldn't it be "clear (FeatureMips3 | FeatureMips2 | FeatureMipsGP64 | FeatureMips1)" ?

matheusalmeida added inline comments.Jun 18 2014, 6:05 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
238

Please see function ClearImpliedBits in SubtargetFeature.cpp. It only clears the features that imply the bit being cleared and not the other way around. It might be clearer for you if you compare the two functions (SetImpliedBits and ClearImpliedBits).

matheusalmeida retitled this revision from [mips] Add assembler support for '.set : 1) 'mips1' 2) 'mips2' 3) 'mips3' 4) 'mips4' 5) 'mips5' 6) 'mips32' 7) 'mips32r6' 8) 'mips64r6' to [mips] Add assembler support for '.set mipsX'..Jun 18 2014, 8:52 AM
matheusalmeida edited edge metadata.
matheusalmeida added a subscriber: Unknown Object (MLST).Jun 18 2014, 10:02 AM

CC'ing the list because there's a small modification to MCSubtargetInfo.h.

This patch adds support to many of the .set mipsX assembler directives that allow an ISA to be selected/deselected in the same translation unit. Example:

.text
.set mips32
// Mips32 ISA available
.set mips64
// Mips64 ISA available

Mips ISAs are defined with respect to older ISAs. For example:

FeatureMips1 : None
FeatureMips2 : FeatureMips1
FeatureMips3 : FeatureMips2 | FeatureMipsGP64
and so on...

One of the issues of disabling a feature is that the function ClearImpliedBits in SubtargetFeature only clears the features that imply the feature being cleared. As an example, clearing the FeatureMips3 will not clear (FeatureMips2 | FeatureMipsGP64 | FeatureMips1).

The idea behind this patch is that every time we select a new ISA, we clear all the bits related to ISAs (updating MCSubtargetInfo) and select the new one as 'setImpliedBits' does the right thing.

Does this make sense ?

Regards,
Matheus

tomatabacu commandeered this revision.Jul 9 2014, 5:36 AM
tomatabacu added a reviewer: matheusalmeida.
tomatabacu added a subscriber: tomatabacu.

Commandeered to finish off the patch and make it ready for commit.

tomatabacu updated this revision to Diff 11190.Jul 9 2014, 5:49 AM
tomatabacu updated this object.

For MipsAsmParser.cpp:
Corrected grammar in comment (double "by") in line 54.

For MipsTargetStreamer.cpp:
Added dummy implementations of the functions introduced by the patch to the MipsTargetStreamer class.
Added "setCanHaveModuleDir(false);" to the definitions of the functions introduced by the patch in both MipsTargetAsmStreamer and MipsTargetELFStreamer classes.

For MipsTargetStreamer.h:
Changed virtual member functions of MipsTargetStreamer introduced by the patch to not be pure virtual functions.

For set-mips-directives-bad.s:
Addressed a concern about the comment.

Rebased to trunk.

tomatabacu updated this revision to Diff 12032.Jul 30 2014, 8:22 AM

Ran clang-format and rebased.

dsanders accepted this revision.Aug 4 2014, 5:26 AM
dsanders edited edge metadata.

LGTM. There is a cleanup that I'd like to be done afterwards though.

At the moment, MipsTargetELFStreamer::emitDirectiveSetMips*() and MipsTargetAsmStreamer::emitDirectiveSetMips*() both call setCanHaveModuleDir(). We should move this call to the base class (MipsTargetStreamer), delete MipsTargetELFStreamer::emitDirectiveSetMips*() since they no longer do anything and have MipsTargetAsmStreamer::emitDirectiveSetMips*() call the base class implementation after emitting the directive.

This revision is now accepted and ready to land.Aug 4 2014, 5:26 AM
dsanders closed this revision.Aug 4 2014, 5:28 AM