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

[Jim Grosbach]

On Feb 4, 2011, at 3:10 PM, Bruno Cardoso Lopes wrote:

> 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).

Excellent. That should allow us to use this for things like restricted range immediates that don't fit nicely into the current general purpose lattice framework.

> 
>> 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.

No specific examples in mind, no. Just a potential ambiguity that I'm not clear on what will happen. It'll very likely be a non-issue in practice.

> 
>> 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.
> 

Ah, OK. I was thinking an empty string was the style used for other stuff there. Faulty memory, then. Whatever matches the existing style works for me.

> 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 :)
> 

I'm fine w/ going ahead w/ committing this.

There's been a lot of talk about how to address issues like this, but nothing formal for long term that I'm aware of. This is a great step up and should solve a lot of the problems. Thanks for working on this!

-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