[llvm-commits] [llvm] r134760 - in /llvm/trunk: lib/Target/ARM/AsmParser/ARMAsmParser.cpp test/MC/ARM/mode-switch.s
Jim Grosbach
grosbach at apple.com
Mon Jul 11 09:12:03 PDT 2011
Cool. Thanks!
-Jim
On Jul 11, 2011, at 9:10 AM, Evan Cheng wrote:
>
>
> On Jul 11, 2011, at 8:11 AM, Jim Grosbach <grosbach at apple.com> wrote:
>
>> Hi Evan,
>>
>> It's really great to have this. It's been a big problem with the ARM assembly handling for a long time. Some comments inline.
>>
>> On Jul 8, 2011, at 3:36 PM, Evan Cheng wrote:
>>
>>> Author: evancheng
>>> Date: Fri Jul 8 17:36:29 2011
>>> New Revision: 134760
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=134760&view=rev
>>> Log:
>>> Add support for ARM / Thumb mode switching with .code 16 and .code 32.
>>>
>>> Added:
>>> llvm/trunk/test/MC/ARM/mode-switch.s
>>> Modified:
>>> llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
>>>
>>> Modified: llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp?rev=134760&r1=134759&r2=134760&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp (original)
>>> +++ llvm/trunk/lib/Target/ARM/AsmParser/ARMAsmParser.cpp Fri Jul 8 17:36:29 2011
>>> @@ -42,7 +42,11 @@
>>>
>>> class ARMAsmParser : public TargetAsmParser {
>>> MCAsmParser &Parser;
>>> - OwningPtr<const MCSubtargetInfo> STI;
>>> + /// STI, ARM_STI, Thumb_STI - Subtarget info for ARM and Thumb modes. STI
>>> + /// points to either ARM_STI or Thumb_STI depending on the mode.
>>> + const MCSubtargetInfo *STI;
>>> + OwningPtr<const MCSubtargetInfo> ARM_STI;
>>> + OwningPtr<const MCSubtargetInfo> Thumb_STI;
>>>
>>> MCAsmParser &getParser() const { return Parser; }
>>> MCAsmLexer &getLexer() const { return Parser.getLexer(); }
>>> @@ -89,10 +93,13 @@
>>> // FIXME: Can tablegen auto-generate this?
>>> return (STI->getFeatureBits() & ARM::ModeThumb) != 0;
>>> }
>>> -
>>> bool isThumbOne() const {
>>> return isThumb() && (STI->getFeatureBits() & ARM::FeatureThumb2) == 0;
>>> }
>>> + void SwitchMode() {
>>> + STI = isThumb() ? ARM_STI.get() : Thumb_STI.get();
>>> + setAvailableFeatures(ComputeAvailableFeatures(STI->getFeatureBits()));
>>> + }
>>>
>>
>> Perhaps better to use a standard setter method here with an explicit parameter indicating which is desired? That would be more consistent with the rest of the code, plus it would make the calling function(s) more explicit without the need for control flow.
>
> I plan to change this to avoid unnecessarily recompute features.
>
>
>>
>> On the nitpicky side, the naming convention changed recently such that function names should always start with a lower-case letter. (i.e., switchMode() rather than SwitchMod()).
>>
>>> /// @name Auto-generated Match Functions
>>> /// {
>>> @@ -129,10 +136,24 @@
>>>
>>> public:
>>> ARMAsmParser(StringRef TT, StringRef CPU, StringRef FS, MCAsmParser &_Parser)
>>> - : TargetAsmParser(), Parser(_Parser),
>>> - STI(ARM_MC::createARMMCSubtargetInfo(TT, CPU, FS)) {
>>> -
>>> + : TargetAsmParser(), Parser(_Parser) {
>>> MCAsmParserExtension::Initialize(_Parser);
>>> +
>>> + STI = ARM_MC::createARMMCSubtargetInfo(TT, CPU, FS);
>>> + // FIXME: Design a better way to create two subtargets with only difference
>>> + // being a feature change.
>>> + if (isThumb()) {
>>> + Thumb_STI.reset(STI);
>>> + assert(TT.startswith("thumb") && "Unexpected Triple string for Thumb!");
>>> + Twine ARM_TT = "arm" + TT.substr(5);
>>> + ARM_STI.reset(ARM_MC::createARMMCSubtargetInfo(ARM_TT.str(), CPU, FS));
>>> + } else {
>>> + ARM_STI.reset(STI);
>>> + assert(TT.startswith("arm") && "Unexpected Triple string for ARM!");
>>> + Twine Thumb_TT = "thumb" + TT.substr(3);
>>> + Thumb_STI.reset(ARM_MC::createARMMCSubtargetInfo(Thumb_TT.str(),CPU, FS));
>>> + }
>>> +
>>
>> I'm not sure I follow this bit. Why is this conditional on the starting mode? Don't we want to unconditionally create both STI's and then conditionally set the starting available features based on the which STI the mode indicates we should use?
>
> This code has completely changed already.
>
> Evan
>
>>
>>> // Initialize the set of available features.
>>> setAvailableFeatures(ComputeAvailableFeatures(STI->getFeatureBits()));
>>> }
>>> @@ -2215,18 +2236,11 @@
>>> return Error(Parser.getTok().getLoc(), "unexpected token in directive");
>>> Parser.Lex();
>>>
>>> - // FIXME: We need to be able switch subtargets at this point so that
>>> - // MatchInstructionImpl() will work when it gets the AvailableFeatures which
>>> - // includes Feature_IsThumb or not to match the right instructions. This is
>>> - // blocked on the FIXME in llvm-mc.cpp when creating the TargetMachine.
>>> - if (Val == 16){
>>> - assert(isThumb() &&
>>> - "switching between arm/thumb not yet suppported via .code 16)");
>>> + if (Val == 16) {
>>> + if (!isThumb()) SwitchMode();
>>> getParser().getStreamer().EmitAssemblerFlag(MCAF_Code16);
>>> - }
>>> - else{
>>> - assert(!isThumb() &&
>>> - "switching between thumb/arm not yet suppported via .code 32)");
>>> + } else {
>>> + if (isThumb()) SwitchMode();
>>> getParser().getStreamer().EmitAssemblerFlag(MCAF_Code32);
>>> }
>>>
>>>
>>> Added: llvm/trunk/test/MC/ARM/mode-switch.s
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/MC/ARM/mode-switch.s?rev=134760&view=auto
>>> ==============================================================================
>>> --- llvm/trunk/test/MC/ARM/mode-switch.s (added)
>>> +++ llvm/trunk/test/MC/ARM/mode-switch.s Fri Jul 8 17:36:29 2011
>>> @@ -0,0 +1,16 @@
>>> +@ Test ARM / Thumb mode switching with .code
>>> +@ RUN: llvm-mc -mcpu=cortex-a8 -triple arm-unknown-unknown -show-encoding < %s | FileCheck %s
>>> +
>>> +.code 16
>>> +
>>> +@ CHECK: add.w r0, r0, r1 @ encoding: [0x01,0x00,0x00,0xeb]
>>> + add.w r0, r0, r1
>>> +
>>> +.code 32
>>> +@ CHECK: add r0, r0, r1 @ encoding: [0x01,0x00,0x80,0xe0]
>>> + add r0, r0, r1
>>> +
>>> +.code 16
>>> +@ CHECK: add r0, r0, r1 @ encoding: [0x40,0x18]
>>> +
>>> + add r0, r0, r1
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
More information about the llvm-commits
mailing list