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

[• tstellarAMD]

Summary

If there was an InstAlias defined for an instruction that had a custom converter, then when the alias was matched, the custom converter was used rather than the converted supplied with the alias.

This is required for some future improvements to the R600 assembler.

[ab]

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

utils/TableGen/AsmMatcherEmitter.cpp

441

and -> an

[• tstellarAMD]

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

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

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.

[ab]

In D9083#161473, @tstellarAMD wrote:

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

[• tstellarAMD]

Here is an updated patch that adds an option to the InstAlias class that lets you choose whether to use the AsmMatchConverter for the result instruction or the converter generated by the InstAlias.

Unlike the first version of the patch, this patch does not change the current behavior.

[• tstellarAMD]

Ping for the updated patch.

[ab]

LGTM, thanks!

utils/TableGen/AsmMatcherEmitter.cpp

441

this is use to determine whether or not to using
->
this is used to determine whether or not to use

443

generated by -> generated for? Might be clearer. (same in Target.td)

include/llvm/Target/Target.td

 
   // Predicates - Predicates that must be true for this to match.
   list<Predicate> Predicates = [];
+
+  // If the instruction specified in Result has defined an AsmMatchConverter
+  // then setting this to 1 will cause the alias to use the AsmMatchConverter
+  // function when converting the OperandVector into an MCInst instead of the
+  // function that is generated by the dag Result.
+  // Setting this to 0 will cause the alias to ignore the Result instruction's
+  // defined AsmMatchConverter and instead use the function generated by the
+  // dag Result.
+  bit UseInstAsmMatchConverter = 1;
 }
 
 //===----------------------------------------------------------------------===//

utils/TableGen/AsmMatcherEmitter.cpp

   /// If this instruction is deprecated in some form.
   bool HasDeprecation;
 
+  /// If this is an alias, this is use to determine whether or not to using
+  /// the conversion function defined by the instruction's AsmMatchConverter
+  /// or to use the function generated by the alias.
+  bool UseInstAsmMatchConverter;
+
   MatchableInfo(const CodeGenInstruction &CGI)
-    : AsmVariantID(0), AsmString(CGI.AsmString), TheDef(CGI.TheDef), DefRec(&CGI) {
+    : AsmVariantID(0), AsmString(CGI.AsmString), TheDef(CGI.TheDef), DefRec(&CGI),
+      UseInstAsmMatchConverter(true) {
   }
 
   MatchableInfo(std::unique_ptr<const CodeGenInstAlias> Alias)
-    : AsmVariantID(0), AsmString(Alias->AsmString), TheDef(Alias->TheDef), DefRec(Alias.release()) {
+    : AsmVariantID(0), AsmString(Alias->AsmString), TheDef(Alias->TheDef),
+      DefRec(Alias.release()),
+      UseInstAsmMatchConverter(
+        TheDef->getValueAsBit("UseInstAsmMatchConverter")) {
   }
 
   ~MatchableInfo() {

     // Check if we have a custom match function.
     std::string AsmMatchConverter =
       II->getResultInst()->TheDef->getValueAsString("AsmMatchConverter");
-    if (!AsmMatchConverter.empty()) {
+    if (!AsmMatchConverter.empty() && II->UseInstAsmMatchConverter) {
       std::string Signature = "ConvertCustom_" + AsmMatchConverter;
       II->ConversionFnKind = Signature;