[llvm-commits] [PATCH] Asm operand parsing improvements

[Bruno Cardoso Lopes]

Hi Jim,

On Fri, Feb 4, 2011 at 8:26 PM, Jim Grosbach <grosbach at apple.com> wrote:
> Hi Bruno,
>
> This looks very cool and is definitely an improvement over the ugly hacks we currently have.
>
> In the ARM parsing, you've placed the call to try to do custom operand parsing after the check for the token being an identifier. Is that necessary? It seems that it could/should be at the head of the ParseOperand() method so it will fire for any operands marked as needing custom parsing, whether they start with an identifier or not.

Agreed, I think it could go in the head of the ParseOperand without
any problem (at least that I'm aware of).

> What happens if for a given opcode, some entries in the table have a custom parser listed for an operand position and others do not? Is there a clear-cut "right answer" for that? If not, should it be allowed at all?

Since it naively tries to match in the following order: opcode,
features, operand index, it's not a problem, unless for some weird
case I didn't think about yet, do you have one in mind? Anyway, this
could be improved over time if we find more interesting cases.

> Nitpick, but shouldn't the initialization of the ParserMethod in Target.td be to the empty string rather than a question mark?

It probably can, I just followed the initialization pattern already
there, but no problem to change.

Jim, Daniel, or someone else from ARM's world, it's ok to commit this?
Are there any other plans or directions for this kind of stuff I
should be aware of?
Thanks for the feedback Jim :)

> On Jan 26, 2011, at 5:21 AM, Bruno Cardoso Lopes wrote:
>
>> Hi,
>>
>> The first attached patch implements support for custom target specific
>> asm parsing of operands. This is useful to avoid adding hacks to
>> AsmParsers whenever something different from an immediate or register
>> appears. The initial motivation was to remove the "mcr" family of
>> hacks from ARMAsmParser, but this is also going to be useful for other
>> ARM system instructions which I'm about to add the correct parsing and
>> encoding once this gets accepted. The second attached patch contains
>> modifications to the ARMAsmParser so that it starts using this
>> mechanism.
>>
>> A quick explanation of how it works (read the first patch for details):
>>
>> A table like the one below is autogenerated for every instruction
>> containing a 'ParserMethod' in its AsmOperandClass:
>>
>> static const OperandMatchEntry OperandMatchTable[20] = {
>>  /* Mnemonic, Operand List Mask, Operand Class, Features */
>>  { "cdp", 29 /* 0, 2, 3, 4 */, MCK_Coproc, Feature_IsThumb|Feature_HasV6 },
>>  { "cdp", 58 /* 1, 3, 4, 5 */, MCK_Coproc, Feature_IsARM },
>>
>> A matcher function very similar (but lot more naive) to
>> MatchInstructionImpl scans the table. After the mnemonic match, the
>> features are checked and if the "to be parsed" operand index is
>> present in the mask, there's a real match. Then, a switch like the one
>> below dispatch the parsing to the custom method provided in
>> 'ParseMethod':
>>
>>  case MCK_Coproc:
>>    return TryParseCoprocessorOperandName(Operands);
>>
>> Ok to commit? Suggestions?
>> Thanks.
>>
>> --
>> Bruno Cardoso Lopes
>> http://www.brunocardoso.cc
>> <asm-custom-operand-parse.patch><arm-operand-parsing.patch>_______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>



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