[TableGen] Temporary hack to add type checking for multiclass template arguments [DO NOT COMMIT]

[Paul-C-Anagnostopoulos]

Summary

Apply this hack patch to a throw-away branch to help you find and fix multiclass template arguments that are of the wrong type. Such arguments have been discovered in the AMDGPU, ARM, MSP430, RISCV, WebAssembly, and X86 targets. The problem may exist in others, also.

See this conversation: https://lists.llvm.org/pipermail/llvm-dev/2021-February/148261.html

[arsenm]

Should probably add a testcase

llvm/lib/TableGen/TGParser.cpp

3511

Invert and reduce indentation?

3516

Braces

[Paul-C-Anagnostopoulos]

This is not the code that will end up in the real TableGen, so I wasn't worried about formatting.

I will write a monster test for all the template argument parsing, checking, and resolving. This is not the only problem I uncovered.

[RKSimon]

Thanks @Paul-C-Anagnostopoulos - I'll take a look at the x86 cases

[RKSimon]

@Paul-C-Anagnostopoulos Please can you rebase and confirm which targets still have build failures?

[Paul-C-Anagnostopoulos]

I will do that later today.

~~ Paul

[Paul-C-Anagnostopoulos]

As of Feb. 5 at 14:30 EST, the following targets still have problems:

AArch, Hexagon, PowerPC, RISCV, Sparc, SystemZ, VE, WebAssembly

[RKSimon]

I've pushed trivial changes to a few targets and created D96205 for the webassembly guys to review - I think that covers the remaining issues raised by this patch.

It'd be great if you could get your real template types patch ready to go soon - I expect the situation to bitrot again quickly without some form of error message.

[Paul-C-Anagnostopoulos]

I'll work as fast as I can, but it's part of a large patch that deals with all three kinds of template arguments. And I'll keep building the targets as I go.

[RKSimon]

Cheers - something that came up on D96205 was whether it would be possible to report when a class was over generalized - when all instances used more specific classes than the parent class that the template specified.

Would it be easy to report when a more specific class could be used?

[Paul-C-Anagnostopoulos]

So you want to know if a multiclass template argument type is a class that was never passed to that multiclass? But if there are two subclasses of that class that are passed to the multiclass, the parent class is the type that must be declared for the template argument.

I think I misunderstand your request.

[RKSimon]

In D95874#2547012, @Paul-C-Anagnostopoulos wrote:

So you want to know if a multiclass template argument type is a class that was never passed to that multiclass? But if there are two subclasses of that class that are passed to the multiclass, the parent class is the type that must be declared for the template argument.

On the patch review there was confusion as to why I could use the Intrinsic subclass in some places but others I had to generalize all the way to SDPatternOperator - some warnings to indicate when we've over-generalized the accepted class might be useful.

[Paul-C-Anagnostopoulos]

Sorry, I still don't understand. In the case of the MSP430, the 'arith' multiclass template argument 'node' needed to be generalized from type SDNode to type SDPatternOperator. This is because some of the uses of 'arith' pass a value that is not an SDNode, but some other subclass of SDPatternOperator. What warning message do you want to see here?

[Paul-C-Anagnostopoulos]

As far as I can tell, the only remaining target with problems is the VE. Does this need to be fixed before I can push the real revision?

[RKSimon]

In D95874#2548516, @Paul-C-Anagnostopoulos wrote:

As far as I can tell, the only remaining target with problems is the VE. Does this need to be fixed before I can push the real revision?

No, go ahead and create the patch and just add the VE people as reviewers so they know that experimental backends will break with the change.

[RKSimon]

In D95874#2548514, @Paul-C-Anagnostopoulos wrote:

Sorry, I still don't understand. In the case of the MSP430, the 'arith' multiclass template argument 'node' needed to be generalized from type SDNode to type SDPatternOperator. This is because some of the uses of 'arith' pass a value that is not an SDNode, but some other subclass of SDPatternOperator. What warning message do you want to see here?

Let's forget about it - it was just a suggestion.

[RKSimon]

@Paul-C-Anagnostopoulos Can we abandon this now?

[Paul-C-Anagnostopoulos]

Yes, I'll abandon it now.

[Paul-C-Anagnostopoulos]

I've reclaimed this patch in order to fix problems in the RISCV target.

@craig.topper
@asb
There are new problems with mismatched multiclass template arguments. Could you find and fix them using this patch? Or I could try.

[craig.topper]

In D95874#2571501, @Paul-C-Anagnostopoulos wrote:

I've reclaimed this patch in order to fix problems in the RISCV target.

@craig.topper
@asb
There are new problems with mismatched multiclass template arguments. Could you find and fix them using this patch? Or I could try.

Looks like RISCV is clean right now. Maybe 065a187f337f2a3204c69c2a73a65aad49a44be1 fixed it?

[Paul-C-Anagnostopoulos]

I'll check it here and then move on to the bigger fun with clang-tblgen.

[Paul-C-Anagnostopoulos]

@t.p.northover

I think the Clang problem might be in this code in arm_mve.td. I've flagged 'top', which is a bit. However, EmitterBase::getCodeForDagArg (in MveEmiiter.cpp) does not handle a bit as argument #2 of this DAG. At least, that's what I gather. I have improved the error message that it produces, which is shown below the code. What I don't understand is why this revision suddenly produces an error where none was produced before. I'll continue to investigate.

multiclass vmovl<bit top> {
  let params = [s8, u8, s16, u16] in {
    def "": Intrinsic<DblVector, (args Vector:$a),
      (extend (unzip $a, top), DblVector, (unsignedflag Scalar))>;
    defm "": IntrinsicMX<DblVector, (args Vector:$a, DblPredicate:$pred),
        (IRInt<"vmovl_predicated", [DblVector, Vector, DblPredicate]>
         $a, (unsignedflag Scalar), **top**, $pred, $inactive)>;
  }
}

error: bad DAG argument type for code generation
note: DAG: (anonymous_336 ?:$a, (unsignedflag Scalar), 0, ?:$pred, ?:$inactive)
note: argument type: bit
note: argument number 2: 0
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.

[Paul-C-Anagnostopoulos]

I will also post the previous note here: https://reviews.llvm.org/D96416

llvm/lib/TableGen/TGParser.cpp

while (true) {
     // Verify that the correct number of template arguments were specified.
     ArrayRef<Init *> TArgs = MC->Rec.getTemplateArgs();
     if (TArgs.size() < TemplateVals.size())
       return Error(SubClassLoc,
                    "more template args specified than multiclass expects");
 
     SubstStack Substs;
     for (unsigned i = 0, e = TArgs.size(); i != e; ++i) {
       if (i < TemplateVals.size()) {
+        // This if block is a hack so we can track down multiclass template
+        // argument values of the wrong type.
+        if (TypedInit *ArgValue = dyn_cast<TypedInit>(TemplateVals[i])) {
+          auto *ArgType = MC->Rec.getValue(TArgs[i])->getType();
+          if (!ArgValue->getCastTo(ArgType))
+            return Error(SubClassLoc,
+                         "Value specified for template argument '" +
+                             TArgs[i]->getAsString() + "' (#" + Twine(i) +
+                             ") is of type " + ArgValue->getType()->getAsString() +
+                             "; expected type " + ArgType->getAsString() + ": " +
+                             ArgValue->getAsString());
+        }
         Substs.emplace_back(TArgs[i], TemplateVals[i]);
+
       } else {
         Init *Default = MC->Rec.getValue(TArgs[i])->getValue();
         if (!Default->isComplete()) {
           return Error(SubClassLoc,
                        "value not specified for template argument #" +
                            Twine(i) + " (" + TArgs[i]->getAsUnquotedString() +
                            ") of multiclass '" + MC->Rec.getNameInitAsString() +
                            "'");