[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 ↗

(On Diff #144811)

Haha, nice!

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

1 ↗

(On Diff #144811)

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 ↗

(On Diff #144811)

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 ↗

(On Diff #145132)

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

3 ↗

(On Diff #145132)

Ditto

1 ↗

(On Diff #144811)

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!

[rtereshin]

Adding comments to the tests as requested.

[qcolombet]

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

1 ↗

(On Diff #145698)

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

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

1 ↗

(On Diff #145698)

Ditto

[rtereshin]

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

1 ↗

(On Diff #145698)

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.

llvm/trunk/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;
 

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

 # RUN: not llc -mtriple=aarch64-- -run-pass=legalizer %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: G_INTTOPTR could only be legal /
 # could be legalized if its destination operand has a pointer type and
 # its source - a scalar type. This is reversed in this test and the
 # legalizer is expected to fail on it with an appropriate error
 # message. Prior to LegalizerInfo::verify AArch64 legalizer had a
 # subtle bug in its definition that caused it to accept the following

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

 # RUN: not llc -mtriple=aarch64-- -run-pass=legalizer %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: G_INTTOPTR could only be legal /
 # could be legalized if its destination operand has a pointer type and
 # its source - a scalar type of an appropriate size. This test meets
 # the requirements for type index 0 (the pointer) and LLT-size
 # requirements for type index 1 (64 bits for AArch64), but has a
 # non-scalar (vector) type for type index 1. The Legalizer is expected

llvm/trunk/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:
 ...