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 ↗

(On Diff #23949)

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 ↗

(On Diff #24848)

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

443 ↗

(On Diff #24848)

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

llvm/trunk/include/llvm/Target/Target.td

class InstAlias<string Asm, dag Result, int Emit = 1> {
   // This determines which order the InstPrinter detects aliases for
   // printing. A larger value makes the alias more likely to be
   // emitted. The Instruction's own definition is notionally 0.5, so 0
   // disables printing and 1 enables it if there are no conflicting aliases.
   int EmitPriority = Emit;
 
   // 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;
 }
 
 //===----------------------------------------------------------------------===//
 // AsmWriter - This class can be implemented by targets that need to customize
 // the format of the .s file writer.
 //
 // Subtargets can have multiple different asmwriters (e.g. AT&T vs Intel syntax
 // on X86 for example).

llvm/trunk/utils/TableGen/AsmMatcherEmitter.cpp

struct MatchableInfo {
   /// ConversionFnKind - The enum value which is passed to the generated
   /// convertToMCInst to convert parsed operands into an MCInst for this
   /// function.
   std::string ConversionFnKind;
 
   /// 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() {
     delete DefRec.dyn_cast<const CodeGenInstAlias*>();
   }
 
   // Two-operand aliases clone from the main matchable, but mark the second
   // operand as a tied operand of the first for purposes of the assembler.
static void emitConvertFuncs(CodeGenTarget &Target, StringRef ClassName,
   OperandConversionKinds.insert("CVT_Reg");
   OperandConversionKinds.insert("CVT_Tied");
   enum { CVT_Done, CVT_Reg, CVT_Tied };
 
   for (auto &II : Infos) {
     // 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;
 
       // Check if we have already generated this signature.
       if (!InstructionConversionKinds.insert(Signature))
         continue;
 
       // Remember this converter for the kind enum.