[GlobalISel][Legalizer] LegalizerInfo verifier: Follow Up

[rtereshin]

Summary

This is a follow up on https://reviews.llvm.org/D46338, the patch contains the following commits:

[GlobalISel][AArch64] LegalizerInfo verifier: Fixing bugs exposed by LegalizerInfo::verify(...)

[GlobalISel][Legalizer] LegalizerInfo verifier: Making LegalizerInfo::verify(...) errors fatal

[GlobalISel][X86] LegalizerInfo verifier: Adding LegalizerInfo::verify(...) call and fixing bugs exposed

[GlobalISel][ARM] LegalizerInfo verifier:  Adding LegalizerInfo::verify(...) call and fixing bugs exposed

[GlobalISel][AMDGPU] LegalizerInfo verifier: Adding LegalizerInfo::verify(...) call for AMDGPU

[GlobalISel][Mips] LegalizerInfo verifier: Adding LegalizerInfo::verify(...) call for Mips

[qcolombet]

lib/Target/AArch64/AArch64LegalizerInfo.cpp

228

Haha, nice!

test/CodeGen/AArch64/GlobalISel/legalize-inttoptr-xfail-2.mir

1–6

Should be converted into a not llc with a comment explaining why it is incorrect.
(Already said it in the other review.)

[rtereshin]

test/CodeGen/AArch64/GlobalISel/legalize-inttoptr-xfail-2.mir

1–6

What's the policy here to decide between XFAIL and not llc (or not opt etc)?

[rtereshin]

Updated the xfail tests to make their purpose clearer.

[qcolombet]

With not llc you can still check specifics in FileCheck whereas with XFAIL you usually don't put checks at all.
In this case, I would expect a not llc + filecheck of some error message.

[rtereshin]

In D46339#1087865, @qcolombet wrote:

With not llc you can still check specifics in FileCheck whereas with XFAIL you usually don't put checks at all.
In this case, I would expect a not llc + filecheck of some error message.

Yes! I've done exactly that, apparently :)

[qcolombet]

test/CodeGen/AArch64/GlobalISel/legalize-inttoptr-xfail-1.mir

6

Could you put the comment back and describe it in a more useful way?
Basically, call out that we are catching a type mismatch, expecting pointer but got scalar.

test/CodeGen/AArch64/GlobalISel/legalize-inttoptr-xfail-2.mir

1–6

I don't think there are any.
Personally I tend to avoid XFAIL at all, because you can have crashes and whatnot and it will still happily pass!

3

Ditto

[rtereshin]

Adding comments to the tests as requested.

[qcolombet]

test/CodeGen/AArch64/GlobalISel/legalize-inttoptr-xfail-1.mir

1

Out-of-curiosity, why is the simplify-mir option required here?

test/CodeGen/AArch64/GlobalISel/legalize-inttoptr-xfail-2.mir

1

Ditto

[rtereshin]

test/CodeGen/AArch64/GlobalISel/legalize-inttoptr-xfail-1.mir

1

It's not required, but I find it useful: if a test fails I often just copy-paste the command line used as reported by llvm-lit quite mechanically to see a broader context than FileCheck included in the error message, and it's nice if it happens to produce a shorten output.

I can stop committing these of course.

[qcolombet]

Yes, please remove these.

Simplify-mir may get smarter and eliminate more stuff and we don't
want to have to patch the check lines.

[rtereshin]

Removing -simplify-mir command line options from run-lines.

lib/CodeGen/GlobalISel/LegalizerInfo.cpp

for (unsigned Opcode = FirstOp; Opcode <= LastOp; ++Opcode) {
     if (!RuleSet.verifyTypeIdxsCoverage(NumTypeIdxs))
       FailedOpcodes.push_back(Opcode);
   }
   if (!FailedOpcodes.empty()) {
     errs() << "The following opcodes have ill-defined legalization rules:";
     for (unsigned Opcode : FailedOpcodes)
       errs() << " " << MII.getName(Opcode);
     errs() << "\n";
+
+    report_fatal_error("ill-defined LegalizerInfo"
+                       ", try -debug-only=legalizer-info for details");
   }
 #endif
 }
 
 #ifndef NDEBUG
 // FIXME: This should be in the MachineVerifier, but it can't use the
 // LegalizerInfo as it's currently in the separate GlobalISel library.
 // Note that RegBankSelected property already checked in the verifier

lib/Target/AArch64/AArch64LegalizerInfo.cpp

AArch64LegalizerInfo::AArch64LegalizerInfo(const AArch64Subtarget &ST) {
   getActionDefinitionsBuilder(G_FCMP)
       .legalFor({{s32, s32}, {s32, s64}})
       .clampScalar(0, s32, s32)
       .clampScalar(1, s32, s64)
       .widenScalarToNextPow2(1);
 
   // Extensions
   getActionDefinitionsBuilder({G_ZEXT, G_SEXT, G_ANYEXT})
-      .legalFor({s1, s8, s16, s32, s64})
-      .maxScalar(0, s64)
-      .widenScalarToNextPow2(0);
+      .legalForCartesianProduct({s8, s16, s32, s64}, {s1, s8, s16, s32});
 
   // FP conversions
   getActionDefinitionsBuilder(G_FPTRUNC).legalFor(
       {{s16, s32}, {s16, s64}, {s32, s64}});
   getActionDefinitionsBuilder(G_FPEXT).legalFor(
       {{s32, s16}, {s64, s16}, {s64, s32}});
 
   // Conversions
getActionDefinitionsBuilder(G_PTRTOINT)
       .legalForCartesianProduct({s1, s8, s16, s32, s64}, {p0})
       .maxScalar(0, s64)
       .widenScalarToNextPow2(0, /*Min*/ 8);
 
   getActionDefinitionsBuilder(G_INTTOPTR)
       .unsupportedIf([&](const LegalityQuery &Query) {
         return Query.Types[0].getSizeInBits() != Query.Types[1].getSizeInBits();
       })
-      .legalFor({s64, p0});
+      .legalFor({{p0, s64}});
 
   // Casts for 32 and 64-bit width type are just copies.
   // Same for 128-bit width type, except they are on the FPR bank.
   getActionDefinitionsBuilder(G_BITCAST)
       // FIXME: This is wrong since G_BITCAST is not allowed to change the
       // number of bits but it's what the previous code described and fixing
       // it breaks tests.
       .legalForCartesianProduct({s1, s8, s16, s32, s64, s128, v16s8, v8s8, v4s8,
                                  v8s16, v4s16, v2s16, v4s32, v2s32, v2s64});
 
   getActionDefinitionsBuilder(G_VASTART).legalFor({p0});
 
   // va_list must be a pointer, but most sized types are pretty easy to handle
   // as the destination.
   getActionDefinitionsBuilder(G_VAARG)
       .customForCartesianProduct({s8, s16, s32, s64, p0}, {p0})
       .clampScalar(0, s8, s64)
       .widenScalarToNextPow2(0, /*Min*/ 8);
 
   if (ST.hasLSE()) {
-    getActionDefinitionsBuilder(G_ATOMIC_CMPXCHG)
-        .legalForCartesianProduct({s8, s16, s32, s64}, {p0});
-  }
-
-  if (ST.hasLSE()) {
     getActionDefinitionsBuilder(G_ATOMIC_CMPXCHG_WITH_SUCCESS)
-        .lowerFor({s8, s16, s32, s64});
+        .lowerForCartesianProduct({s8, s16, s32, s64}, {s1}, {p0});
 
     getActionDefinitionsBuilder(
         {G_ATOMICRMW_XCHG, G_ATOMICRMW_ADD, G_ATOMICRMW_SUB, G_ATOMICRMW_AND,
          G_ATOMICRMW_OR, G_ATOMICRMW_XOR, G_ATOMICRMW_MIN, G_ATOMICRMW_MAX,
-         G_ATOMICRMW_UMIN, G_ATOMICRMW_UMAX})
+         G_ATOMICRMW_UMIN, G_ATOMICRMW_UMAX, G_ATOMIC_CMPXCHG})
         .legalForCartesianProduct({s8, s16, s32, s64}, {p0});
   }
 
   // Merge/Unmerge
   for (unsigned Op : {G_MERGE_VALUES, G_UNMERGE_VALUES}) {
     unsigned BigTyIdx = Op == G_MERGE_VALUES ? 0 : 1;
     unsigned LitTyIdx = Op == G_MERGE_VALUES ? 1 : 0;
 

lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

getActionDefinitionsBuilder(Op)
                        [](const LegalityQuery &Query) {
                          return std::make_pair(
                            1, Query.Types[1].getElementType());
                        });
 
   }
 
   computeTables();
+  verify(*ST.getInstrInfo());
 }

lib/Target/ARM/ARMLegalizerInfo.cpp

for (unsigned Op : {G_SREM, G_UREM}) {
     if (ST.hasDivideInARMMode())
       setAction({Op, s32}, Lower);
     else if (AEABI(ST))
       setAction({Op, s32}, Custom);
     else
       setAction({Op, s32}, Libcall);
   }
 
-  getActionDefinitionsBuilder({G_SEXT, G_ZEXT, G_ANYEXT}).legalFor({s32});
+  getActionDefinitionsBuilder({G_SEXT, G_ZEXT, G_ANYEXT})
+      .legalForCartesianProduct({s32}, {s1, s8, s16});
 
   getActionDefinitionsBuilder(G_INTTOPTR).legalFor({{p0, s32}});
   getActionDefinitionsBuilder(G_PTRTOINT).legalFor({{s32, p0}});
 
   getActionDefinitionsBuilder({G_ASHR, G_LSHR, G_SHL}).legalFor({s32});
 
   getActionDefinitionsBuilder(G_GEP).legalFor({{p0, s32}});
 
if (!ST.useSoftFloat() && ST.hasVFP2()) {
     getActionDefinitionsBuilder(G_FCMP).customForCartesianProduct({s1},
                                                                   {s32, s64});
 
     if (AEABI(ST))
       setFCmpLibcallsAEABI();
     else
       setFCmpLibcallsGNU();
 
-    getActionDefinitionsBuilder(G_FPEXT).libcallFor({s64, s32});
-    getActionDefinitionsBuilder(G_FPTRUNC).libcallFor({s32, s64});
+    getActionDefinitionsBuilder(G_FPEXT).libcallFor({{s64, s32}});
+    getActionDefinitionsBuilder(G_FPTRUNC).libcallFor({{s32, s64}});
 
     getActionDefinitionsBuilder({G_FPTOSI, G_FPTOUI})
         .libcallForCartesianProduct({s32}, {s32, s64});
     getActionDefinitionsBuilder({G_SITOFP, G_UITOFP})
         .libcallForCartesianProduct({s32, s64}, {s32});
   }
 
   if (!ST.useSoftFloat() && ST.hasVFP4())
     getActionDefinitionsBuilder(G_FMA).legalFor({s32, s64});
   else
     getActionDefinitionsBuilder(G_FMA).libcallFor({s32, s64});
 
   getActionDefinitionsBuilder({G_FREM, G_FPOW}).libcallFor({s32, s64});
 
   computeTables();
+  verify(*ST.getInstrInfo());
 }
 
 void ARMLegalizerInfo::setFCmpLibcallsAEABI() {
   // FCMP_TRUE and FCMP_FALSE don't need libcalls, they should be
   // default-initialized.
   FCmp32Libcalls.resize(CmpInst::LAST_FCMP_PREDICATE + 1);
   FCmp32Libcalls[CmpInst::FCMP_OEQ] = {
       {RTLIB::OEQ_F32, CmpInst::BAD_ICMP_PREDICATE}};

lib/Target/Mips/MipsLegalizerInfo.cpp

 //===- MipsLegalizerInfo.cpp ------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 /// \file
 /// This file implements the targeting of the Machinelegalizer class for Mips.
 /// \todo This should be generated by TableGen.
 //===----------------------------------------------------------------------===//
 
 #include "MipsLegalizerInfo.h"
+#include "MipsTargetMachine.h"
 
 using namespace llvm;
 
 MipsLegalizerInfo::MipsLegalizerInfo(const MipsSubtarget &ST) {
   using namespace TargetOpcode;
 
   const LLT s32 = LLT::scalar(32);
 
   getActionDefinitionsBuilder(G_ADD).legalFor({s32});
 
   computeTables();
+  verify(*ST.getInstrInfo());
 }

lib/Target/X86/X86LegalizerInfo.cpp

for (unsigned MemOp : {G_LOAD, G_STORE})
     setLegalizeScalarToDifferentSizeStrategy(MemOp, 0,
        narrowToSmallerAndWidenToSmallest);
   setLegalizeScalarToDifferentSizeStrategy(
       G_GEP, 1, widenToLargerTypesUnsupportedOtherwise);
   setLegalizeScalarToDifferentSizeStrategy(
       G_CONSTANT, 0, widenToLargerTypesAndNarrowToLargest);
 
   computeTables();
+  verify(*STI.getInstrInfo());
 }
 
 void X86LegalizerInfo::setLegalizerInfo32bit() {
 
   const LLT p0 = LLT::pointer(0, TM.getPointerSizeInBits(0));
   const LLT s1 = LLT::scalar(1);
   const LLT s8 = LLT::scalar(8);
   const LLT s16 = LLT::scalar(16);
void X86LegalizerInfo::setLegalizerInfo32bit() {
   setAction({G_GEP, p0}, Legal);
   setAction({G_GEP, 1, s32}, Legal);
 
   if (!Subtarget.is64Bit()) {
     getActionDefinitionsBuilder(G_PTRTOINT)
         .legalForCartesianProduct({s1, s8, s16, s32}, {p0})
         .maxScalar(0, s32)
         .widenScalarToNextPow2(0, /*Min*/ 8);
-    getActionDefinitionsBuilder(G_INTTOPTR).legalFor({s32, p0});
+    getActionDefinitionsBuilder(G_INTTOPTR).legalFor({{p0, s32}});
 
     // Shifts and SDIV
     getActionDefinitionsBuilder({G_SHL, G_LSHR, G_ASHR, G_SDIV})
         .legalFor({s8, s16, s32})
         .clampScalar(0, s8, s32);
   }
 
   // Control-flow
for (unsigned MemOp : {G_LOAD, G_STORE})
     setAction({MemOp, s64}, Legal);
 
   // Pointer-handling
   setAction({G_GEP, 1, s64}, Legal);
   getActionDefinitionsBuilder(G_PTRTOINT)
       .legalForCartesianProduct({s1, s8, s16, s32, s64}, {p0})
       .maxScalar(0, s64)
       .widenScalarToNextPow2(0, /*Min*/ 8);
-  getActionDefinitionsBuilder(G_INTTOPTR).legalFor({s64, p0});
+  getActionDefinitionsBuilder(G_INTTOPTR).legalFor({{p0, s64}});
 
   // Constants
   setAction({TargetOpcode::G_CONSTANT, s64}, Legal);
 
   // Extensions
   for (unsigned extOp : {G_ZEXT, G_SEXT, G_ANYEXT}) {
     setAction({extOp, s64}, Legal);
   }

test/CodeGen/AArch64/GlobalISel/legalize-inttoptr-xfail-1.mir

 # RUN: not llc -mtriple=aarch64-- -run-pass=legalizer -simplify-mir %s -o - 2>&1 | FileCheck %s
 # REQUIRES: asserts
-# XFAIL: *
-
-# This is to demonstrate what kind of bugs we're missing w/o some kind of
-# validation for LegalizerInfo.
 
 # CHECK:      LLVM ERROR: unable to legalize instruction:
 # CHECK-SAME: %{{[0-9]+}}:_(s64) = G_INTTOPTR %{{[0-9]+}}:_(p0)
 # CHECK-SAME: (in function: broken)
 
 ---
 name:            broken
 alignment:       2

test/CodeGen/AArch64/GlobalISel/legalize-inttoptr-xfail-2.mir

 # RUN: not llc -mtriple=aarch64-- -run-pass=legalizer -simplify-mir %s -o - 2>&1 | FileCheck %s
 # REQUIRES: asserts
-# XFAIL: *
-
-# This is to demonstrate what kind of bugs we're missing w/o some kind of
-# validation for LegalizerInfo.
 
 # CHECK:      LLVM ERROR: unable to legalize instruction:
 # CHECK-SAME: %{{[0-9]+}}:_(p0) = G_INTTOPTR %{{[0-9]+}}:_(<4 x s16>)
 # CHECK-SAME: (in function: broken)
 
 ---
 name:            broken
 alignment:       2
 tracksRegLiveness: true
 registers:
   - { id: 0, class: _ }
   - { id: 1, class: _ }

test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir

 # RUN: llc -mtriple=aarch64-- -run-pass=legalizer %s \
-# RUN:     -mcpu=cortex-a75 -o - 2>&1 | FileCheck %s --check-prefixes=CHECK       --match-full-lines
+# RUN:     -mcpu=cortex-a75 -o - 2>&1 | FileCheck %s --check-prefixes=CHECK
 
 # RUN: llc -mtriple=aarch64-- -run-pass=legalizer %s -debug-only=legalizer-info \
-# RUN:     -mcpu=cortex-a75 -o - 2>&1 | FileCheck %s --check-prefixes=CHECK,DEBUG --match-full-lines
+# RUN:     -mcpu=cortex-a75 -o - 2>&1 | FileCheck %s --check-prefixes=CHECK,DEBUG
 
 # REQUIRES: asserts
 
 # The main purpose of this test is to make sure we don't over-relax
 # LegalizerInfo validation and loose its ability to catch bugs.
 #
 # Watch out for every "SKIPPED: user-defined predicate detected" in the
 # check-lines below and keep each and every one of them justified.

 #
 # DEBUG-NEXT: G_MERGE_VALUES (opcode {{[0-9]+}}): 2 type indices
 # DEBUG:      .. type index coverage check SKIPPED: user-defined predicate detected
 #
 # DEBUG-NEXT: G_PTRTOINT (opcode {{[0-9]+}}): 2 type indices
 # DEBUG:      .. the first uncovered type index: 2, OK
 #
 # DEBUG-NEXT: G_INTTOPTR (opcode {{[0-9]+}}): 2 type indices
-# DEBUG:      .. the first uncovered type index: 1, FAIL
+# DEBUG:      .. the first uncovered type index: 2, OK
 #
 # DEBUG-NEXT: G_BITCAST (opcode {{[0-9]+}}): 2 type indices
 # DEBUG:      .. the first uncovered type index: 2, OK
 #
 # DEBUG-NEXT: G_LOAD (opcode {{[0-9]+}}): 2 type indices
 # DEBUG:      .. type index coverage check SKIPPED: user-defined predicate detected
 #
 # DEBUG-NEXT: G_SEXTLOAD (opcode {{[0-9]+}}): 2 type indices
 # DEBUG:      .. type index coverage check SKIPPED: user-defined predicate detected
 #
 # DEBUG-NEXT: G_ZEXTLOAD (opcode {{[0-9]+}}): 2 type indices
 # DEBUG:      .. type index coverage check SKIPPED: user-defined predicate detected
 #
 # DEBUG-NEXT: G_STORE (opcode {{[0-9]+}}): 2 type indices
 # DEBUG:      .. type index coverage check SKIPPED: user-defined predicate detected
 #
 # DEBUG-NEXT: G_ATOMIC_CMPXCHG_WITH_SUCCESS (opcode {{[0-9]+}}): 3 type indices
-# DEBUG:      .. the first uncovered type index: 1, FAIL
+# DEBUG:      .. the first uncovered type index: 3, OK
 #
 # DEBUG-NEXT: G_ATOMIC_CMPXCHG (opcode {{[0-9]+}}): 2 type indices
 # DEBUG:      .. the first uncovered type index: 2, OK
 #
 # DEBUG-NEXT: G_ATOMICRMW_XCHG (opcode {{[0-9]+}}): 2 type indices
 # DEBUG:      .. the first uncovered type index: 2, OK
 #
 # DEBUG-NEXT: G_ATOMICRMW_ADD (opcode {{[0-9]+}}): 2 type indices

 #
 # DEBUG-NEXT: G_INTRINSIC (opcode {{[0-9]+}}): 0 type indices
 # DEBUG:      .. type index coverage check SKIPPED: no rules defined
 #
 # DEBUG-NEXT: G_INTRINSIC_W_SIDE_EFFECTS (opcode {{[0-9]+}}): 0 type indices
 # DEBUG:      .. type index coverage check SKIPPED: no rules defined
 #
 # DEBUG-NEXT: G_ANYEXT (opcode {{[0-9]+}}): 2 type indices
-# DEBUG:      .. the first uncovered type index: 1, FAIL
+# DEBUG:      .. the first uncovered type index: 2, OK
 #
 # DEBUG-NEXT: G_TRUNC (opcode {{[0-9]+}}): 2 type indices
 # DEBUG:      .. type index coverage check SKIPPED: no rules defined
 #
 # DEBUG-NEXT: G_CONSTANT (opcode {{[0-9]+}}): 1 type index
 # DEBUG:      .. the first uncovered type index: 1, OK
 #
 # DEBUG-NEXT: G_FCONSTANT (opcode {{[0-9]+}}): 1 type index
 # DEBUG:      .. the first uncovered type index: 1, OK
 #
 # DEBUG-NEXT: G_VASTART (opcode {{[0-9]+}}): 1 type index
 # DEBUG:      .. the first uncovered type index: 1, OK
 #
 # DEBUG-NEXT: G_VAARG (opcode {{[0-9]+}}): 2 type indices
 # DEBUG:      .. the first uncovered type index: 2, OK
 #
 # DEBUG-NEXT: G_SEXT (opcode {{[0-9]+}}): 2 type indices
-# DEBUG:      .. the first uncovered type index: 1, FAIL
+# DEBUG:      .. the first uncovered type index: 2, OK
 #
 # DEBUG-NEXT: G_ZEXT (opcode {{[0-9]+}}): 2 type indices
-# DEBUG:      .. the first uncovered type index: 1, FAIL
+# DEBUG:      .. the first uncovered type index: 2, OK
 #
 # DEBUG-NEXT: G_SHL (opcode {{[0-9]+}}): 1 type index
 # DEBUG:      .. the first uncovered type index: 1, OK
 #
 # DEBUG-NEXT: G_LSHR (opcode {{[0-9]+}}): 1 type index
 # DEBUG:      .. the first uncovered type index: 1, OK
 #
 # DEBUG-NEXT: G_ASHR (opcode {{[0-9]+}}): 1 type index

 # DEBUG:      .. type index coverage check SKIPPED: no rules defined
 #
 # DEBUG-NEXT: G_SHUFFLE_VECTOR (opcode {{[0-9]+}}): 3 type indices
 # DEBUG:      .. type index coverage check SKIPPED: no rules defined
 #
 # DEBUG-NEXT: G_BSWAP (opcode {{[0-9]+}}): 1 type index
 # DEBUG:      .. the first uncovered type index: 1, OK
 
-# CHECK: The following opcodes have ill-defined legalization rules: G_INTTOPTR G_ATOMIC_CMPXCHG_WITH_SUCCESS G_ANYEXT G_SEXT G_ZEXT
+# CHECK-NOT: ill-defined
 
 ---
 name: dummy
 body: |
   bb.0:
 ...