[PATCH] Allow encoded 8-bit floating point constants in ARM vmov instructions

Jim Grosbach grosbach at apple.com
Tue Dec 17 13:58:14 PST 2013


On Dec 17, 2013, at 12:15 PM, David Peixotto <dpeixott at codeaurora.org> wrote:

>>>> Why do we want fconstf/fconstd aliases? Those are pre-unified syntax
>>>> instructions, which we're trying to remove from LLVM, not add.
>>> 
>>> Mainly because I see these instructions still appearing in code and
>>> gcc accepts them.  We already support a number of aliases for
>>> pre-unified syntax (e.g. flds, fldd). I count at least 40 in
>> lib/Target/ARM/ARMInstrVFP.td.
>> 
>> All of which I would very much like to remove. In retrospect, it was an
>> error to add the ones we (I) did. It was convenient at the time, but the
>> wrong precedent to set.
>> 
>>> Is there a reason not to support them? I think they maps fairly
>>> cleanly to the vmov.f32/vmov.f64 instructions.
>> 
>> It's explicitly adding support for an assembly syntax that has been
>> obsoleted by ARM for over 6 years now and which ARM has indicated they
>> have no interest in supporting. Adding more support for divided syntax
>> increases the maintenance cost of LLVM's ARM assembly parser, which is
>> already a bit overcomplicated. Projects still using divided syntax should
>> be fixed to use unified syntax instead.
> 
> The arm and gcc assmblers both accept the fconstd alias, even in the context
> of unified syntax files. While I agree with you that projects should update
> their source code I don't always have the leverage to make that happen :)
> 
> Perhaps we can put the issue of adding an fconstd alias aside for a moment
> and just address the specific fix in this patch. The current code for
> parsing 8-bit floating point constants is broken in two ways:
> 
> 1. It converts the literal to a double, but the isFPImm() code expects to
> find a float so it is not recognized as an fp immediate by the asm matcher.

Yeah, that’s bogus. Sounds like you already tracked this down to a refactoring gone awry, which makes sense. Unfortunate that we didn’t have tests to catch it at the time, but it’ll be good to have those added.

> 2. It uses the wrong APFloat api to create the immediate which causes us to
> drop the non-integer part of the floating point constant.
> 
> The patch I submitted fixes these two issues. Currently, we get a parse
> error on this code
> 
>  vmov.f32 s0, #0x70
> 
> but it looks like that is quite by accident (due to issue 1 above). Do we
> want to support writing these constants or should we only allow
> floating-point literals (e.g. #1.0 instead of #0x70)?

What does the ARM documentation indicate? Either way, you’re right that we should clean up the implementation so that if we reject or accept the different forms of constants, we do so intentionally and not inadvertently.

-Jim



More information about the llvm-commits mailing list