This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, ARM 1/7] Consolidate Thumb-1/Thumb-2 ISA detection


On 23/12/15 06:43, Thomas Preud'homme wrote:
>> From: binutils-owner@sourceware.org [mailto:binutils-
>> owner@sourceware.org] On Behalf Of Richard Earnshaw (lists)
>> Sent: Friday, December 18, 2015 7:55 PM
>>
>> non_v6t2_wide_only_insn should be rewritten to be
>> v6t2_wide_only_insn
>> with its return values giving the opposite sense.  I really don't like
>> functions that directly imply a negative in their name as the logic
>> becomes very confusing when the result is then inverted.  I note that
>> every single use of this function in the patch uses "!non_...".
> 
> It was actually a positive statement since the negative for that function would be instructions that are not T32 or that are part of v6t2. That of course shows how confusing was the function name and its comment. Hopefully, this should be addressed in this new version (please note that the function changes its name in patch 3/7 for ARMv8-M Baseline support).
> 
>>
>> Otherwise this is OK.
>>
>> I'll note for the record here that the use of Thumb-1 and Thumb-2 is now
>> somewhat anachronistic.  It would be better if the internal variables
>> and terminology could be cleaned up to talk about T16 and T32 encodings
>> (I think that's also somewhat cleaner than wide and narrow).  However,
>> that's a more general cleanup patch and doesn't specifically apply to
>> the changes here.
> 
> Should be better here as well, at least in terms of naming and comments. Of course, one of the two uses for this function is related to deciding the value of Tag_THUMB_ISA_use.
> 
> New ChangeLog entries are as follow:
> 
> 
> *** gas/ChangeLog ***
> 
> 2015-12-22  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>         * config/tc-arm.c (move_or_literal_pool): Check mov.w, mvm and movw
>         availability against arm_ext_v6t2 instead of checking arm_arch_t2,
>         fixing comments along the way.
>         (handle_it_state): Check arm_ext_v6t2 instead of arm_arch_t2 to
>         generate IT instruction.
>         (t1_isa_t32_only_insn): New function.
>         (md_assemble): Use above new function to check for invalid wide
>         instruction for CPU Thumb ISA and to determine what Thumb extension
>         bit is necessary for that instruction.
>         (md_apply_fix): Use arm_ext_v6t2 instead of arm_arch_t2 to decide if
>         branch is out of range.
> 
> 
> *** include/opcode/ChangeLog ***
> 
> 2015-12-10  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>         * arm.h (ARM_ARCH_THUMB2): Add comment explaining its meaning and
>         remove extension bit not including any Thumb-2 instruction.
> 

OK.

R.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]