[MachineVerifier][GlobalISel] Verifying generic extends and truncates

[rtereshin]

Summary

Making sure we don't truncate / extend pointers, don't try to change
vector topology or bitcast vectors to scalars or back, and most
importantly, don't extend to a smaller type or truncate to a large
one.

While crafting a test for this change with a more or less decent code coverage I noticed that our MIR Parser is not able to parse vector pointer LLTs. We do quite explicitly support their representation in LLT class, and there is a direct counterpart in LLVM IR. Also it's easy to imagine a target actually sporting them (with some sort of vector scatter / gather stores / loads. So the test ended up depending on a fix for MIR Parser planned as a separate commit:

[MIRParser][GlobalISel] Parsing vector pointer types (<M x pA>)

MIParser wasn't able to parse LLTs like `<4 x p0>`, fixing that.

This diff includes both commits together.

[qcolombet]

test/CodeGen/AMDGPU/GlobalISel/regbankselect-trunc.mir

15

Haha, this is already paying back :)

[rtereshin]

Thank you, @qcolombet!

The parser part of this got a conflict with recently pushed https://reviews.llvm.org/D45948 (which got bug-fixed a bit in https://reviews.llvm.org/D46491), I'm going to resolve the conflict a tad later. I hope I can assume the revision is still accepted afterwards.

Best,
Roman

test/CodeGen/AMDGPU/GlobalISel/regbankselect-trunc.mir

15

;-)

[rtereshin]

I have updated the MIParser::parseLowLevelType part of the patch (as before, within a separate commit) resolving conflicts with Allow register class names in the form of integer/scalar and Improve error checking for typed immediate operands

lib/CodeGen/MIRParser/MIParser.cpp

if (Token.is(MIToken::ScalarType)) {
     Ty = LLT::pointer(AS, DL.getPointerSizeInBits(AS));
     lex();
     return false;
   }
 
   // Now we're looking for a vector.
   if (Token.isNot(MIToken::less))
     return error(Loc,
-                 "expected unsized, pN, sN or <N x sM> for GlobalISel type");
-
+                 "expected sN, pA, <M x sN>, or <M x pA> for GlobalISel type");
   lex();
 
   if (Token.isNot(MIToken::IntegerLiteral))
-    return error(Loc, "expected <N x sM> for vctor type");
+    return error(Loc, "expected <M x sN> or <M x pA> for vector type");
   uint64_t NumElements = Token.integerValue().getZExtValue();
   lex();
 
   if (Token.isNot(MIToken::Identifier) || Token.stringValue() != "x")
-    return error(Loc, "expected '<N x sM>' for vector type");
+    return error(Loc, "expected <M x sN> or <M x pA> for vector type");
   lex();
 
-  if (Token.isNot(MIToken::ScalarType))
-    return error(Loc, "expected '<N x sM>' for vector type");
-  uint64_t ScalarSize = APSInt(Token.range().drop_front()).getZExtValue();
+  if (Token.is(MIToken::ScalarType))
+    Ty = LLT::scalar(APSInt(Token.range().drop_front()).getZExtValue());
+  else if (Token.is(MIToken::PointerType)) {
+    const DataLayout &DL = MF.getDataLayout();
+    unsigned AS = APSInt(Token.range().drop_front()).getZExtValue();
+    Ty = LLT::pointer(AS, DL.getPointerSizeInBits(AS));
+  } else
+    return error(Loc, "expected <M x sN> or <M x pA> for vector type");
   lex();
 
   if (Token.isNot(MIToken::greater))
-    return error(Loc, "expected '<N x sM>' for vector type");
+    return error(Loc, "expected <M x sN> or <M x pA> for vector type");
   lex();
 
-  Ty = LLT::vector(NumElements, ScalarSize);
+  Ty = LLT::vector(NumElements, Ty);
   return false;
 }
 
 bool MIParser::parseTypedImmediateOperand(MachineOperand &Dest) {
   assert(Token.is(MIToken::IntegerType));
   auto Loc = Token.location();
   lex();
   if (Token.isNot(MIToken::IntegerLiteral))

lib/CodeGen/MachineVerifier.cpp

if (!DstTy.isValid() ||
                          return false;
                        return true;
                      }))
       report("Generic Instruction G_PHI has operands with incompatible/missing "
              "types",
              MI);
     break;
   }
+  case TargetOpcode::G_SEXT:
+  case TargetOpcode::G_ZEXT:
+  case TargetOpcode::G_ANYEXT:
+  case TargetOpcode::G_TRUNC:
+  case TargetOpcode::G_FPEXT:
+  case TargetOpcode::G_FPTRUNC: {
+    // Number of operands and presense of types is already checked (and
+    // reported in case of any issues), so no need to report them again. As
+    // we're trying to report as many issues as possible at once, however, the
+    // instructions aren't guaranteed to have the right number of operands or
+    // types attached to them at this point
+    assert(MCID.getNumOperands() == 2 && "Expected 2 operands G_*{EXT,TRUNC}");
+    if (MI->getNumOperands() < MCID.getNumOperands())
+      break;
+    LLT DstTy = MRI->getType(MI->getOperand(0).getReg());
+    LLT SrcTy = MRI->getType(MI->getOperand(1).getReg());
+    if (!DstTy.isValid() || !SrcTy.isValid())
+      break;
+
+    LLT DstElTy = DstTy.isVector() ? DstTy.getElementType() : DstTy;
+    LLT SrcElTy = SrcTy.isVector() ? SrcTy.getElementType() : SrcTy;
+    if (DstElTy.isPointer() || SrcElTy.isPointer())
+      report("Generic extend/truncate can not operate on pointers", MI);
+
+    if (DstTy.isVector() != SrcTy.isVector()) {
+      report("Generic extend/truncate must be all-vector or all-scalar", MI);
+      // Generally we try to report as many issues as possible at once, but in
+      // this case it's not clear what should we be comparing the size of the
+      // scalar with: the size of the whole vector or its lane. Instead of
+      // making an arbitrary choice and emitting not so helpful message, let's
+      // avoid the extra noise and stop here.
+      break;
+    }
+    if (DstTy.isVector() && DstTy.getNumElements() != SrcTy.getNumElements())
+      report("Generic vector extend/truncate must preserve number of lanes",
+             MI);
+    unsigned DstSize = DstElTy.getSizeInBits();
+    unsigned SrcSize = SrcElTy.getSizeInBits();
+    switch (MI->getOpcode()) {
+    default:
+      if (DstSize <= SrcSize)
+        report("Generic extend has destination type no larger than source", MI);
+      break;
+    case TargetOpcode::G_TRUNC:
+    case TargetOpcode::G_FPTRUNC:
+      if (DstSize >= SrcSize)
+        report("Generic truncate has destination type no smaller than source",
+               MI);
+      break;
+    }
+    break;
+  }
   case TargetOpcode::COPY: {
     if (foundErrors)
       break;
     const MachineOperand &DstOp = MI->getOperand(0);
     const MachineOperand &SrcOp = MI->getOperand(1);
     LLT DstTy = MRI->getType(DstOp.getReg());
     LLT SrcTy = MRI->getType(SrcOp.getReg());
     if (SrcTy.isValid() && DstTy.isValid()) {

test/CodeGen/AMDGPU/GlobalISel/regbankselect-trunc.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 # RUN: llc -march=amdgcn -mcpu=fiji -run-pass=regbankselect -global-isel %s -verify-machineinstrs -o - -regbankselect-fast | FileCheck %s
 # RUN: llc -march=amdgcn -mcpu=fiji -run-pass=regbankselect -global-isel %s -verify-machineinstrs -o - -regbankselect-greedy | FileCheck %s
 
 ---
 name: trunc_i64_to_i32_s
 legalized: true
 
 body: |
   bb.0:
     liveins: $sgpr0_sgpr1
     ; CHECK-LABEL: name: trunc_i64_to_i32_s
-    ; CHECK: [[COPY:%[0-9]+]]:sgpr(s32) = COPY $sgpr0
-    ; CHECK: [[TRUNC:%[0-9]+]]:sgpr(s32) = G_TRUNC [[COPY]](s32)
-    %0:_(s32) = COPY $sgpr0
+    ; CHECK: [[COPY:%[0-9]+]]:sgpr(s64) = COPY $sgpr0
+    ; CHECK: [[TRUNC:%[0-9]+]]:sgpr(s32) = G_TRUNC [[COPY]](s64)
+    %0:_(s64) = COPY $sgpr0_sgpr1
     %1:_(s32) = G_TRUNC %0
 ...
 
 ---
 name: trunc_i64_to_i32_v
 legalized: true
 
 body: |
   bb.0:
     liveins: $vgpr0_vgpr1
     ; CHECK-LABEL: name: trunc_i64_to_i32_v
-    ; CHECK: [[COPY:%[0-9]+]]:vgpr(s32) = COPY $vgpr0
-    ; CHECK: [[TRUNC:%[0-9]+]]:vgpr(s32) = G_TRUNC [[COPY]](s32)
-    %0:_(s32) = COPY $vgpr0
+    ; CHECK: [[COPY:%[0-9]+]]:vgpr(s64) = COPY $vgpr0
+    ; CHECK: [[TRUNC:%[0-9]+]]:vgpr(s32) = G_TRUNC [[COPY]](s64)
+    %0:_(s64) = COPY $vgpr0_vgpr1
     %1:_(s32) = G_TRUNC %0
 ...

test/CodeGen/X86/verifier-generic-extend-truncate.mir

+# RUN: not llc -o - %s -mtriple=x86_64-- -verify-machineinstrs -run-pass=none 2>&1 | FileCheck %s
+
+# CHECK: Bad machine code: Generic extend/truncate can not operate on pointers
+# CHECK-NEXT: - function:    bad_generic_extends_and_truncates
+# CHECK-NEXT: - basic block: %bb.1
+# CHECK-NEXT: - instruction: %t_p:_(s32) = G_TRUNC %p:_(p0)
+
+# CHECK: Bad machine code: Generic extend/truncate must be all-vector or all-scalar
+# CHECK-NEXT: - function:    bad_generic_extends_and_truncates
+# CHECK-NEXT: - basic block: %bb.2
+# CHECK-NEXT: - instruction: %se_i32:_(<2 x s64>) = G_SEXT %i32:_(s32)
+
+# CHECK: Bad machine code: Generic vector extend/truncate must preserve number of lanes
+# CHECK-NEXT: - function:    bad_generic_extends_and_truncates
+# CHECK-NEXT: - basic block: %bb.3
+# CHECK-NEXT: - instruction: %ze_v2i32:_(<4 x s64>) = G_ZEXT %v2i32:_(<2 x s32>)
+
+# CHECK: Bad machine code: Generic extend has destination type no larger than source
+# CHECK-NEXT: - function:    bad_generic_extends_and_truncates
+# CHECK-NEXT: - basic block: %bb.4
+# CHECK-NEXT: - instruction: %ae_i32:_(s32) = G_ANYEXT %i32:_(s32)
+
+# CHECK: Bad machine code: Generic truncate has destination type no smaller than source ***
+# CHECK-NEXT: - function:    bad_generic_extends_and_truncates
+# CHECK-NEXT: - basic block: %bb.5
+# CHECK-NEXT: - instruction: %ft_f32:_(s64) = G_FPTRUNC %f32:_(s32)
+
+
+# CHECK: Bad machine code: Generic extend/truncate can not operate on pointers
+# CHECK-NEXT: - function:    bad_generic_extends_and_truncates
+# CHECK-NEXT: - basic block: %bb.6
+# CHECK-NEXT: - instruction: %ze_v2i128:_(<4 x p0>) = G_ZEXT %v2i128:_(<2 x s128>)
+
+# CHECK: Bad machine code: Generic vector extend/truncate must preserve number of lanes
+# CHECK-NEXT: - function:    bad_generic_extends_and_truncates
+# CHECK-NEXT: - basic block: %bb.6
+# CHECK-NEXT: - instruction: %ze_v2i128:_(<4 x p0>) = G_ZEXT %v2i128:_(<2 x s128>)
+
+# CHECK: Bad machine code: Generic extend has destination type no larger than source
+# CHECK-NEXT: - function:    bad_generic_extends_and_truncates
+# CHECK-NEXT: - basic block: %bb.6
+# CHECK-NEXT: - instruction: %ze_v2i128:_(<4 x p0>) = G_ZEXT %v2i128:_(<2 x s128>)
+
+
+# CHECK: Bad machine code: Generic extend/truncate can not operate on pointers
+# CHECK-NEXT: - function:    bad_generic_extends_and_truncates
+# CHECK-NEXT: - basic block: %bb.6
+# CHECK-NEXT: - instruction: %fe_v2f128:_(p0) = G_FPEXT %v2f128:_(<2 x s128>)
+
+# CHECK: Bad machine code: Generic extend/truncate must be all-vector or all-scalar
+# CHECK-NEXT: - function:    bad_generic_extends_and_truncates
+# CHECK-NEXT: - basic block: %bb.6
+# CHECK-NEXT: - instruction: %fe_v2f128:_(p0) = G_FPEXT %v2f128:_(<2 x s128>)
+
+---
+name:              bad_generic_extends_and_truncates
+tracksRegLiveness: true
+body:              |
+  bb.0:
+    liveins: $rdi, $esi, $rdx, $xmm0, $ymm1, $ymm2
+
+    %p:_(p0) = COPY $rdi
+    %i32:_(s32) = COPY $esi
+    %v2i32:_(<2 x s32>) = COPY $rdx
+    %f32:_(s32) = COPY $xmm0
+    %v2i128:_(<2 x s128>) = COPY $ymm1
+    %v2f128:_(<2 x s128>) = COPY $ymm2
+
+  bb.1:
+    %t_p:_(s32) = G_TRUNC %p
+
+  bb.2:
+    %se_i32:_(<2 x s64>) = G_SEXT %i32
+
+  bb.3:
+    %ze_v2i32:_(<4 x s64>) = G_ZEXT %v2i32
+
+  bb.4:
+    %ae_i32:_(s32) = G_ANYEXT %i32
+
+  bb.5:
+    %ft_f32:_(s64) = G_FPTRUNC %f32
+
+  bb.6:
+    %ze_v2i128:_(<4 x p0>) = G_ZEXT %v2i128
+    %fe_v2f128:_(p0) = G_FPEXT %v2f128
+...