[PATCH] TableGen: AsmMatcherEmitter: Don't use custom converters for instruction aliases

[Ahmed Bougacha]

In http://reviews.llvm.org/D9083#161473, @tstellarAMD wrote:

> In http://reviews.llvm.org/D9083#161432, @ab wrote:
>
> > I can see this being useful, but I don't know these parts very well, so, just to make sure I understand this correctly:  someone might need to have an InstAlias-specific AsmMatchConverter, someday, right?  And they could restore the current behavior - which I can see being useful as well - by setting that to the same converter as the aliasee?  Also, when you say the "converter supplied with the alias", that would be the tblgen-erated converter, right?
> >
> > If that makes no sense, I'm not qualified to review;  if it does, LGTM ;)
>
>
> I think I understand what you are saying about an InstAlias-specific AsmMatchConverter, but let me explain the issue to make sure we are both on the same page.
>
> There are three ways to specify an AsmMatchConverter (the function that converts from OperandVector to MCInst):
>
> 1. Do nothing, tablegen will generate one for you.
> 2. Set Instruction::AsmMatchConverter to the name of a function (e.g. cvtFoo) and then implement that function in your target's AsmParser
> 3. Define an InstAlias, which will cause TableGen to generate a custom AsmMatchConverter based on the InstAlias.
>
>   This patch attempts to fix what I consider to be a bug, which is that if you do #2, then you can never use that instruction in an InstAlias, because TableGen will generate a call to your custom AsmMatchConverter (e.g. cvtFoo) rather than to the AsmMatchConverter that is generated based on the InstAlias.


Agreed.

> In your comment, you point out the fact that someone may want to use the custom AsmMatchConverter (e.g. cvtFoo) for an InstAlias.  I had originally thought this would never be useful, but after reconsidering, I think it would potentially be useful in the case where you have an InstAlias where the mnemonic and MCInst are different e.g.

> 

>   InstAlias <(shl $dst, $src0, 1), (mul GPR:$dst, GPR:$src0, 2)>;


Yep, I had something like that in mind.  I don't think you need to implement it though, since it would be untestable/untested;  you decide!

-Ahmed

> So, it turns out you are qualified to review the patch ;)

> 

> I think the correct solution would be to add a bit to the InstAlias class that is used to determine which AsmMatchConverter to use (#2 or #3).  I will make that change to this patch.





REPOSITORY
  rL LLVM

http://reviews.llvm.org/D9083

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/