[TableGen][AsmMatcherEmitter] Remove boolean 'Hack' parameter

[sdesmalen]

Summary

An instruction alias should be able to specify the same
(tied) operand twice in the asm string if the asm parser makes
sure the tied operand constraint is satisfied.

[apazos]

Thanks Daniel for pushing this patch.
I tested it internally with my test cases and it works fine.
I also built all targets and did a 'ninja check-all' and I did not detect any new failures.
So I think no target breaks if we remove this Hack flag. Did you confirm it?

Now with this issue fixed, I want to raise another related issue (with tied operands) in AsmWriterEmitter::EmitPrintAliasInstruction.

Note that code bails out if number of operands in the AsmString is higher than the number of operands in the ResultString:

// Don't emit the alias if it has more operands than what it's aliasing.
   if (NumResultOps < CountNumOperands(CGA.AsmString, Variant))
     continue;

So if you have:
Asmstring: "op $rd, $rd, $rs2 ->number of operands is 3
ResultString "newop $rd, $rs2" where rd is tied with the other rs1 input operand, the number of operands is 2.

I think here we should either count number of different operands in the AsmString or add the number of tied operands to the ResultString total number.

[apazos]

utils/TableGen/AsmMatcherEmitter.cpp

1121

change << "ignoring instruction with tied operand ' to << "instruction with tied operand ' or remove the warning...

[sdesmalen]

In D41728#968954, @apazos wrote:

I tested it internally with my test cases and it works fine.
I also built all targets and did a 'ninja check-all' and I did not detect any new failures.
So I think no target breaks if we remove this Hack flag. Did you confirm it?

We have specific tests for SVE for which this change is needed which all pass. Also, doing a check-all found no issues for other targets, so I think we should be good.

Now with this issue fixed, I want to raise another related issue (with tied operands) in AsmWriterEmitter::EmitPrintAliasInstruction.
Note that code bails out if number of operands in the AsmString is higher than the number of operands in the ResultString:

Are you saying that you have a case where you expect the Alias to have more operands than the actual instruction? Because I can't directly think of a reason why having a duplicate operand would be useful in an instruction alias?

Thanks Daniel for pushing this patch.

I'm not Daniel ;)

utils/TableGen/AsmMatcherEmitter.cpp

1121

I think the 'ignoring' is there in the message because the instruction is actually ignored (see the caller of validate()), which is actually a good thing to warn about.

[apazos]

Hi Sander, sorry for the name confusion.
The callers of validate() skip the instruction in one case, but not in the other. So you are trying to say the message is for the case it is skipped.
What is your alias test case? In my case the AsmString has repeated operands, but the result instruction it maps to is defined with tied operands.
Anyways, this change looks fine for me, if you want to move to validate/merge.

[fhahn]

I do not think just removing the error message for the hack is the right thing to do.

With this patch, it is possible to define instruction aliases like (AArch64 example)

def : InstAlias<"mov $dst, $src, $src, $src", (ORRWrs GPR32:$dst, WZR, GPR32:$src, 0), 2>;

And now

bar:
        mov w1, w0, w2, w3
        mov w1, w0, w0, w0

gets encoded to the following by llvm-mc:

bar:
	orr	w1, wzr, w0             // encoding: [0xe1,0x03,0x00,0x2a]
	orr	w1, wzr, w0             // encoding: [0xe1,0x03,0x00,0x2a]

If we want to support instruction aliases with tied operands, it seems like we have to add tied constraints for strings with repeated operands automatically or provide a way to specify the alias as "mov $dst, $src1, $src2, $src3" with "$src1 = $src2 = $src3"

[fhahn]

utils/TableGen/AsmMatcherEmitter.cpp

1119

This warning will only be shown if tablegen is run with -debug, which is not the case for normal debug builds (AFAIK)

[apazos]

Hi Florian, I think when you define an alias with repeated operands you need to additionally define checkEarlyTargetMatchPredicate in your target to make sure that a if you are trying to build an "orr" instruction coming from a "mov" instruction, the mov instruction coming in has the right tied operands. Otherwise you reject the match. This warning causes failure in an build with asserts enabled.

[sdesmalen]

I'm abandoning this change as I have a new patch that addresses this properly: D42293.

In short, I think the error for aliases can be removed if and only if the repeated operand actually maps to a 'tied' operand that is checked during assembling.
So, for the example that @fhahn gave:

def : InstAlias<"mov $dst, $src, $src, $src", (ORRWrs GPR32:$dst, WZR, GPR32:$src, 0), 2>;

The second $src is not linked to a tied operand, so repeating $src should cause TableGen to give an error. To demonstrate this, I added another SVE patch that defines an alias with a repeated operand here: D42295.
Adding another reference to $Zdn in the InstAlias asm string would also cause an error by TableGen.

While investigating this, I found that the generic tied operands constraint checks as implemented in D41446 did not properly cover aliases, this new patch fixes that.
@apazos, note that these constraint checks are a generalization of 'checkEarlyTargetMatchPredicate' which only seems to be implemented by Mips, so I think that can be removed when D42293 is merged.

[fhahn]

Thanks Sander! I'll have a look over the weekend

utils/TableGen/AsmMatcherEmitter.cpp

struct MatchableInfo {
 
   void initialize(const AsmMatcherInfo &Info,
                   SmallPtrSetImpl<Record*> &SingletonRegisters,
                   AsmVariantInfo const &Variant,
                   bool HasMnemonicFirst);
 
   /// validate - Return true if this matchable is a valid thing to match against
   /// and perform a bunch of validity checking.
-  bool validate(StringRef CommentDelimiter, bool Hack) const;
+  bool validate(StringRef CommentDelimiter) const;
 
   /// findAsmOperand - Find the AsmOperand with the specified name and
   /// suboperand index.
   int findAsmOperand(StringRef N, int SubOpIdx) const {
     auto I = find_if(AsmOperands, [&](const AsmOperand &Op) {
       return Op.SrcOpName == N && Op.SubOpIdx == SubOpIdx;
     });
     return (I != AsmOperands.end()) ? I - AsmOperands.begin() : -1;
default:
       InTok = true;
       break;
     }
   }
   if (InTok && Prev != String.size())
     addAsmOperand(String.substr(Prev), IsIsolatedToken);
 }
 
-bool MatchableInfo::validate(StringRef CommentDelimiter, bool Hack) const {
+bool MatchableInfo::validate(StringRef CommentDelimiter) const {
   // Reject matchables with no .s string.
   if (AsmString.empty())
     PrintFatalError(TheDef->getLoc(), "instruction with empty asm string");
 
   // Reject any matchables with a newline in them, they should be marked
   // isCodeGenOnly if they are pseudo instructions.
   if (AsmString.find('\n') != std::string::npos)
     PrintFatalError(TheDef->getLoc(),
bool MatchableInfo::validate(StringRef CommentDelimiter) const {
   std::set<std::string> OperandNames;
   for (const AsmOperand &Op : AsmOperands) {
     StringRef Tok = Op.Token;
     if (Tok[0] == '$' && Tok.find(':') != StringRef::npos)
       PrintFatalError(TheDef->getLoc(),
                       "matchable with operand modifier '" + Tok +
                       "' not supported by asm matcher.  Mark isCodeGenOnly!");
 
-    // Verify that any operand is only mentioned once.
-    // We reject aliases and ignore instructions for now.
+    // If an operand is mentioned more than once, issue a warning.
     if (Tok[0] == '$' && !OperandNames.insert(Tok).second) {
-      if (!Hack)
-        PrintFatalError(TheDef->getLoc(),
-                        "ERROR: matchable with tied operand '" + Tok +
-                        "' can never be matched!");
-      // FIXME: Should reject these.  The ARM backend hits this with $lane in a
-      // bunch of instructions.  It is unclear what the right answer is.
       DEBUG({
         errs() << "warning: '" << TheDef->getName() << "': "
                << "ignoring instruction with tied operand '"
                << Tok << "'\n";
       });
       return false;
     }
   }
 
   return true;
 }
for (const CodeGenInstruction *CGI : Target.getInstructionsByEnumValue()) {
         continue;
 
       auto II = llvm::make_unique<MatchableInfo>(*CGI);
 
       II->initialize(*this, SingletonRegisters, Variant, HasMnemonicFirst);
 
       // Ignore instructions which shouldn't be matched and diagnose invalid
       // instruction definitions with an error.
-      if (!II->validate(CommentDelimiter, true))
+      if (!II->validate(CommentDelimiter))
         continue;
 
       Matchables.push_back(std::move(II));
     }
 
     // Parse all of the InstAlias definitions and stick them in the list of
     // matchables.
     std::vector<Record*> AllInstAliases =
for (unsigned i = 0, e = AllInstAliases.size(); i != e; ++i) {
       if (!V.empty() && V != Variant.Name)
         continue;
 
       auto II = llvm::make_unique<MatchableInfo>(std::move(Alias));
 
       II->initialize(*this, SingletonRegisters, Variant, HasMnemonicFirst);
 
       // Validate the alias definitions.
-      II->validate(CommentDelimiter, false);
+      II->validate(CommentDelimiter);
 
       Matchables.push_back(std::move(II));
     }
   }
 
   // Build info for the register classes.
   buildRegisterClasses(SingletonRegisters);