[llvm-commits] [PATCH] arm BFI instruction!

[Bruno Cardoso Lopes]

Hi Renato,

On Tue, Jan 18, 2011 at 7:31 AM, Renato Golin <Renato.Golin at arm.com> wrote:
>
> On 01/17/11 18:13, Bruno Cardoso Lopes wrote:
> > ping,
> > I have more patches that fix other encoding and asm parsing issues for
> > other instructions, should I send all for revision or is it better to
> > commit directly (with testcases) and leave then for post-commit
> > review?
>
> Hi Bruno,
>
> Sorry for the delay. I have a few questions...
>
> First, can you describe the difference between the current model and the
> new (alternative) notation? Maybe some examples? It's good to keep it in
> the commit log.

The "alternative" one is the form accepted by gas while the other is
probably the darwin one. The former is documented in the ARM docs,
from where I got the syntax, the current model I have no idea where
it's documented, but from the code it seems to accept a (~mask)
describing the bits which should be copied.

> There seem to be some indentation changes in the table gen, can you
> refrain from committing them just now? After the change you can add as
> "cosmetic changes", but right now it'll give the wrong impression.

I'm not following, the only indentation change in the patch regards a
'let Constraints = "$src = $Rd" in' which is now  used for two
instructions descriptions instead of one.

> You have added a Requires<[IsARM, HasV6T2]> to the ARM version of the
> instruction, would be good to do the same with the Thumb version.

The thumb version already has a IsThumb2 predicate, which based on the
former BFI description already there. This seems to be enough.

> Also, this is personal, but BFI_Alt doesn't seem a good name. This is
> not an alternative encoding, this is the documented in ARM ARM, so I
> guess it could either get a real name (like 4-param-BFI) or the other
> get its name changed.

I agree with you, I'm just following a name convention used for a few
instruction in the X86 be. I don't think "4-param-BFI" is good either,
but we need a better name indeed.

> Finally, what is the isAsmParserOnly flag? Does it mean that that
> instruction can only be parsed by a disassembler? Would be good to have
> it accessible from the codegen, especially because it's not a
> pseudo-instruction. But that's up to the back-end people to decide.

One of the versions have to be isAsmParserOnly in order to avoid
decoding conflicts. This would be better described as an instruction
alias, but IIRC the current tablegen support isn't enough to describe
the difference between both versions. See the X86 back-end for some
other examples.

> My tuppence,

Thanks for the feedback! :)

--
Bruno Cardoso Lopes
http://www.brunocardoso.cc