[GISel]: Fix generation of illegal COPYs (of different sizes) during CallLowering

[aditya_nandakumar]

Summary

We end up creating COPY's that are either truncating/extending and this should be illegal.
Eg %0(s8) = COPY %w0
Instead, we generate TRUNCs/ANY_EXT and let the optimizer deal with it later.
%tmp(s32) = COPY %w0
%0(s8) = G_TRUNC %tmp(s32)

I've updated the AArch64 backend, I'd appreciate it if people more familiar with X86/ARM can update it as well(post feedback).

Looking forward to your comments.

[igorb]

X86 backend part - https://reviews.llvm.org/D37678
Could you please merge into you patch.
Regards.

[qcolombet]

Hi Aditya,

Could you add a check in the verifier too?

Cheers,
-Quentin

[aditya_nandakumar]

Added verifier check for catching simple cases (non subreg indices for now).

[aditya_nandakumar]

Adding the verifier check has resulted in 5, 12, and 43 failures in ARM, AArch64, and X86 respectively which needs to be fixed.

[aditya_nandakumar]

Splitting of the verifier check into https://reviews.llvm.org/D37775.

[qcolombet]

The AArch64 part looks good to me.

include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h

549

You can split the patch for those, when you commit.

670

Ditto.

lib/CodeGen/MachineVerifier.cpp

52

This change is problematic because we add a dependency from CodeGen to GlobalISel

953

Using the type of the vregs, MachineRegisterInfo::getType, should eliminate the dependency on RegisterBankInfo.

[aditya_nandakumar]

lib/CodeGen/MachineVerifier.cpp

953

This part has moved to https://reviews.llvm.org/D37775
MRI->getType would only give me valid types for generic virtual registers.
For cases such as %0(s16) = COPY %W0, I'd still need to get the size of W0. If dependency is the issue, I can certainly do what RegisterBankInfo::getSizeInBits() is doing, but that's duplicating work.

[qcolombet]

lib/CodeGen/MachineVerifier.cpp

953

I would push the interesting part of getSizeInBits from RegisterBankInfo to TargetRegisterInfo and have RegisterBankInfo::getSizeInBits uses TargetRegisterInfo::getSizeInBits to avoid the duplication while having the necessary functionality available for the verifier.

[aditya_nandakumar]

lib/CodeGen/MachineVerifier.cpp

953

Quentin - I refactored getSizeInBits in the other review.

[aditya_nandakumar]

Merged X86 part from https://reviews.llvm.org/D37678
Thanks Igor.

[aditya_nandakumar]

Updated ARM-irtranslator.ll test to account for extendRegister emitting G_ANYEXT.

Now, all of the backends pass tests - but LowerFormalArguments still generates wrong (truncating) copies. This will likely be an issue when https://reviews.llvm.org/D37775 lands
Could someone familiar with ARM codebase implement that?

[rovka]

In D37640#886384, @aditya_nandakumar wrote:

Could someone familiar with ARM codebase implement that?

I'll look into it.

[rovka]

Hi Aditya,

I uploaded the ARM changes here: https://reviews.llvm.org/differential/diff/118037/

As a sidenote, this caused trouble with some of my integration tests because now the legalize combiner finds all sorts of opportunities to replace trunc + ext with shifts, which weren't implemented on ARM yet (they are now). I don't think the combiner should be introducing instructions without first checking if they are legal, maybe you can consider fixing that in the future.

[aditya_nandakumar]

Thanks for the patch Diana.
Also regarding the side node, I'll be looking into that soon.

[aditya_nandakumar]

Updated with Diana's patch for ARM.

[aditya_nandakumar]

Can I get an LGTM for this if there are no other comments/concerns?

[qcolombet]

LGTM

[aditya_nandakumar]

Committed in r315240

include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h

public:
   /// Build and insert \p Res<def> = COPY Op
   ///
   /// Register-to-register COPY sets \p Res to \p Op.
   ///
   /// \pre setBasicBlock or setMI must have been called.
   ///
   /// \return a MachineInstrBuilder for the newly created instruction.
   MachineInstrBuilder buildCopy(unsigned Res, unsigned Op);
+  template <typename DstType, typename SrcType>
+  MachineInstrBuilder buildCopy(DstType &&Res, SrcType &&Src) {
+    return buildCopy(getDestFromArg(Res), getRegFromArg(Src));
+  }
 
   /// Build and insert `Res<def> = G_LOAD Addr, MMO`.
   ///
   /// Loads the value stored at \p Addr. Puts the result in \p Res.
   ///
   /// \pre setBasicBlock or setMI must have been called.
   /// \pre \p Res must be a generic virtual register.
   /// \pre \p Addr must be a generic virtual register with pointer type.
public:
   ///
   /// \pre setBasicBlock or setMI must have been called.
   /// \pre \p Res must be a generic virtual register with scalar or vector type.
   /// \pre \p Op must be a generic virtual register with scalar or vector type.
   /// \pre \p Res must be smaller than \p Op
   ///
   /// \return The newly created instruction.
   MachineInstrBuilder buildTrunc(unsigned Res, unsigned Op);
+  template <typename DstType, typename SrcType>
+  MachineInstrBuilder buildTrunc(DstType &&Res, SrcType &&Src) {
+    return buildTrunc(getDestFromArg(Res), getRegFromArg(Src));
+  }
 
   /// Build and insert a \p Res = G_ICMP \p Pred, \p Op0, \p Op1
   ///
   /// \pre setBasicBlock or setMI must have been called.
 
   /// \pre \p Res must be a generic virtual register with scalar or
   ///      vector type. Typically this starts as s1 or <N x s1>.
   /// \pre \p Op0 and Op1 must be generic virtual registers with the

lib/CodeGen/GlobalISel/CallLowering.cpp

unsigned CallLowering::ValueHandler::extendRegister(unsigned ValReg,
   LLT LocTy{VA.getLocVT()};
   switch (VA.getLocInfo()) {
   default: break;
   case CCValAssign::Full:
   case CCValAssign::BCvt:
     // FIXME: bitconverting between vector types may or may not be a
     // nop in big-endian situations.
     return ValReg;
-  case CCValAssign::AExt:
+  case CCValAssign::AExt: {
     assert(!VA.getLocVT().isVector() && "unexpected vector extend");
-    // Otherwise, it's a nop.
-    return ValReg;
+    auto MIB = MIRBuilder.buildAnyExt(LocTy, ValReg);
+    return MIB->getOperand(0).getReg();
+  }
   case CCValAssign::SExt: {
     unsigned NewReg = MRI.createGenericVirtualRegister(LocTy);
     MIRBuilder.buildSExt(NewReg, ValReg);
     return NewReg;
   }
   case CCValAssign::ZExt: {
     unsigned NewReg = MRI.createGenericVirtualRegister(LocTy);
     MIRBuilder.buildZExt(NewReg, ValReg);
     return NewReg;
   }
   }
   llvm_unreachable("unable to extend register");
 }

lib/CodeGen/MachineVerifier.cpp

 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetInstrInfo.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetRegisterInfo.h"
 #include "llvm/Target/TargetSubtargetInfo.h"
+#include "llvm/CodeGen/GlobalISel/RegisterBankInfo.h"
 using namespace llvm;
 
 namespace {
   struct MachineVerifier {
 
     MachineVerifier(Pass *pass, const char *b) :
       PASS(pass),
       Banner(b)
if (!DstTy.isValid() ||
                          return false;
                        return true;
                      }))
       report("Generic Instruction G_PHI has operands with incompatible/missing "
              "types",
              MI);
     break;
   }
+  case TargetOpcode::COPY: {
+    const MachineOperand &DstOp = MI->getOperand(0);
+    const MachineOperand &SrcOp = MI->getOperand(1);
+    unsigned DstSize =
+        RegisterBankInfo::getSizeInBits(DstOp.getReg(), *MRI, *TRI);
+    unsigned SrcSize =
+        RegisterBankInfo::getSizeInBits(SrcOp.getReg(), *MRI, *TRI);
+    if (DstSize != SrcSize)
+      // Catch only obvious cases not involving subregs for now.
+      if (!DstOp.getSubReg() && !SrcOp.getSubReg())
+        report("Copy Instruction is illegal with mismatching sizes", MI);
+    break;
+  }
   case TargetOpcode::STATEPOINT:
     if (!MI->getOperand(StatepointOpers::IDPos).isImm() ||
         !MI->getOperand(StatepointOpers::NBytesPos).isImm() ||
         !MI->getOperand(StatepointOpers::NCallArgsPos).isImm())
       report("meta operands to STATEPOINT not constant!", MI);
     break;
 
     auto VerifyStackMapConstant = [&](unsigned Offset) {

lib/Target/AArch64/AArch64CallLowering.cpp

unsigned getStackAddress(uint64_t Size, int64_t Offset,
     MIRBuilder.buildFrameIndex(AddrReg, FI);
     StackUsed = std::max(StackUsed, Size + Offset);
     return AddrReg;
   }
 
   void assignValueToReg(unsigned ValVReg, unsigned PhysReg,
                         CCValAssign &VA) override {
     markPhysRegUsed(PhysReg);
+    switch (VA.getLocInfo()) {
+    default:
       MIRBuilder.buildCopy(ValVReg, PhysReg);
-    // FIXME: assert extension
+      break;
+    case CCValAssign::LocInfo::SExt:
+    case CCValAssign::LocInfo::ZExt:
+    case CCValAssign::LocInfo::AExt: {
+      auto Copy = MIRBuilder.buildCopy(LLT{VA.getLocVT()}, PhysReg);
+      MIRBuilder.buildTrunc(ValVReg, Copy);
+      break;
+    }
+    }
   }
 
   void assignValueToAddress(unsigned ValVReg, unsigned Addr, uint64_t Size,
                             MachinePointerInfo &MPO, CCValAssign &VA) override {
     auto MMO = MIRBuilder.getMF().getMachineMemOperand(
         MPO, MachineMemOperand::MOLoad | MachineMemOperand::MOInvariant, Size,
         0);
     MIRBuilder.buildLoad(ValVReg, Addr, *MMO);

test/CodeGen/AArch64/GlobalISel/arm64-callingconv.ll

 ; CHECK: [[ONE:%[0-9]+]](s16) = G_CONSTANT i16 1
 ; CHECK: [[FOUR:%[0-9]+]](s32) = G_CONSTANT i32 4
 ; CHECK: [[F_ONE:%[0-9]+]](s32) = G_FCONSTANT float 1.000000e+00
 ; CHECK: [[TWO:%[0-9]+]](s64) = G_FCONSTANT double 2.000000e+00
 
 ; CHECK: %w0 = COPY [[ANSWER]]
 ; CHECK: %d0 = COPY [[D_ONE]]
 ; CHECK: %x1 = COPY [[TWELVE]]
-; CHECK: %w2 = COPY [[THREE]](s8)
-; CHECK: %w3 = COPY [[ONE]](s16)
+; CHECK: [[THREE_TMP:%[0-9]+]](s32) = G_ANYEXT [[THREE]]
+; CHECK: %w2 = COPY [[THREE_TMP]](s32)
+; CHECK: [[ONE_TMP:%[0-9]+]](s32) = G_ANYEXT [[ONE]]
+; CHECK: %w3 = COPY [[ONE_TMP]](s32)
 ; CHECK: %w4 = COPY [[FOUR]](s32)
 ; CHECK: %s1 = COPY [[F_ONE]](s32)
 ; CHECK: %d2 = COPY [[TWO]](s64)
 declare void @varargs(i32, double, i64, ...)
 define void @test_varargs() {
   call void(i32, double, i64, ...) @varargs(i32 42, double 1.0, i64 12, i8 3, i16 1, i32 4, float 1.0, double 2.0)
   ret void
 }

test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll

 ; FALLBACK-WITH-REPORT-OUT: fmov d0, #1.0
 define [1 x double] @constant() {
   ret [1 x double] [double 1.0]
 }
 
   ; The key problem here is that we may fail to create an MBB referenced by a
   ; PHI. If so, we cannot complete the G_PHI and mustn't try or bad things
   ; happen.
-; FALLBACK-WITH-REPORT-ERR: remark: <unknown>:0:0: cannot select: G_STORE %vreg5, %vreg2; mem:ST4[%addr] GPR:%vreg5,%vreg2 (in function: pending_phis)
+; FALLBACK-WITH-REPORT-ERR: remark: <unknown>:0:0: cannot select: G_STORE %vreg6, %vreg2; mem:ST4[%addr] GPR:%vreg6,%vreg2 (in function: pending_phis)
 ; FALLBACK-WITH-REPORT-ERR: warning: Instruction selection used fallback path for pending_phis
 ; FALLBACK-WITH-REPORT-OUT-LABEL: pending_phis:
 define i32 @pending_phis(i1 %tst, i32 %val, i32* %addr) {
   br i1 %tst, label %true, label %false
 
 end:
   %res = phi i32 [%val, %true], [42, %false]
   ret i32 %res

test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll

define void @test_insertvalue_agg(%struct.nested* %addr, {i8, i32}* %addr2) {
   %smallstruct = load {i8, i32}, {i8, i32}* %addr2
   %struct = load %struct.nested, %struct.nested* %addr
   %res = insertvalue %struct.nested %struct, {i8, i32} %smallstruct, 1
   store %struct.nested %res, %struct.nested* %addr
   ret void
 }
 
 ; CHECK-LABEL: name: test_select
-; CHECK: [[TST:%[0-9]+]](s1) = COPY %w0
+; CHECK: [[TST_C:%[0-9]+]](s32) = COPY %w0
+; CHECK: [[TST:%[0-9]+]](s1) = G_TRUNC [[TST_C]]
 ; CHECK: [[LHS:%[0-9]+]](s32) = COPY %w1
 ; CHECK: [[RHS:%[0-9]+]](s32) = COPY %w2
 ; CHECK: [[RES:%[0-9]+]](s32) = G_SELECT [[TST]](s1), [[LHS]], [[RHS]]
 ; CHECK: %w0 = COPY [[RES]]
 define i32 @test_select(i1 %tst, i32 %lhs, i32 %rhs) {
   %res = select i1 %tst, i32 %lhs, i32 %rhs
   ret i32 %res
 }
 
 ; CHECK-LABEL: name: test_select_ptr
-; CHECK: [[TST:%[0-9]+]](s1) = COPY %w0
+; CHECK: [[TST_C:%[0-9]+]](s32) = COPY %w0
+; CHECK: [[TST:%[0-9]+]](s1) = G_TRUNC [[TST_C]]
 ; CHECK: [[LHS:%[0-9]+]](p0) = COPY %x1
 ; CHECK: [[RHS:%[0-9]+]](p0) = COPY %x2
 ; CHECK: [[RES:%[0-9]+]](p0) = G_SELECT [[TST]](s1), [[LHS]], [[RHS]]
 ; CHECK: %x0 = COPY [[RES]]
 define i8* @test_select_ptr(i1 %tst, i8* %lhs, i8* %rhs) {
   %res = select i1 %tst, i8* %lhs, i8* %rhs
   ret i8* %res
 }
 
 ; CHECK-LABEL: name: test_select_vec
-; CHECK: [[TST:%[0-9]+]](s1) = COPY %w0
+; CHECK: [[TST_C:%[0-9]+]](s32) = COPY %w0
+; CHECK: [[TST:%[0-9]+]](s1) = G_TRUNC [[TST_C]]
 ; CHECK: [[LHS:%[0-9]+]](<4 x s32>) = COPY %q0
 ; CHECK: [[RHS:%[0-9]+]](<4 x s32>) = COPY %q1
 ; CHECK: [[RES:%[0-9]+]](<4 x s32>) = G_SELECT [[TST]](s1), [[LHS]], [[RHS]]
 ; CHECK: %q0 = COPY [[RES]]
 define <4 x i32> @test_select_vec(i1 %tst, <4 x i32> %lhs, <4 x i32> %rhs) {
   %res = select i1 %tst, <4 x i32> %lhs, <4 x i32> %rhs
   ret <4 x i32> %res
 }
; CHECK: BL $memmove, csr_aarch64_aapcs, implicit-def %lr, implicit %sp, implicit %x0, implicit %x1, implicit %x2
   call void @llvm.memmove.p0i8.p0i8.i64(i8* %dst, i8* %src, i64 %size, i32 1, i1 0)
   ret void
 }
 
 declare void @llvm.memset.p0i8.i64(i8*, i8, i64, i32 %align, i1 %volatile)
 define void @test_memset(i8* %dst, i8 %val, i64 %size) {
 ; CHECK-LABEL: name: test_memset
 ; CHECK: [[DST:%[0-9]+]](p0) = COPY %x0
-; CHECK: [[SRC:%[0-9]+]](s8) = COPY %w1
+; CHECK: [[SRC_C:%[0-9]+]](s32) = COPY %w1
+; CHECK: [[SRC:%[0-9]+]](s8) = G_TRUNC [[SRC_C]]
 ; CHECK: [[SIZE:%[0-9]+]](s64) = COPY %x2
 ; CHECK: %x0 = COPY [[DST]]
-; CHECK: %w1 = COPY [[SRC]]
+; CHECK: [[SRC_TMP:%[0-9]+]](s32) = G_ANYEXT [[SRC]]
+; CHECK: %w1 = COPY [[SRC_TMP]]
 ; CHECK: %x2 = COPY [[SIZE]]
 ; CHECK: BL $memset, csr_aarch64_aapcs, implicit-def %lr, implicit %sp, implicit %x0, implicit %w1, implicit %x2
   call void @llvm.memset.p0i8.i64(i8* %dst, i8 %val, i64 %size, i32 1, i1 0)
   ret void
 }
 
 declare i64 @llvm.objectsize.i64(i8*, i1)
 declare i32 @llvm.objectsize.i32(i8*, i1)

test/CodeGen/AArch64/GlobalISel/call-translator.ll

define void @test_multiple_args(i64 %in) {
   call void @multiple_args_callee(i32 42, i64 %in)
   ret void
 }
 
 
 ; CHECK-LABEL: name: test_struct_formal
 ; CHECK: [[DBL:%[0-9]+]](s64) = COPY %d0
 ; CHECK: [[I64:%[0-9]+]](s64) = COPY %x0
-; CHECK: [[I8:%[0-9]+]](s8) = COPY %w1
+; CHECK: [[I8_C:%[0-9]+]](s32) = COPY %w1
+; CHECK: [[I8:%[0-9]+]](s8) = G_TRUNC [[I8_C]]
 ; CHECK: [[ADDR:%[0-9]+]](p0) = COPY %x2
 
 ; CHECK: [[UNDEF:%[0-9]+]](s192) = G_IMPLICIT_DEF
 ; CHECK: [[ARG0:%[0-9]+]](s192) = G_INSERT [[UNDEF]], [[DBL]](s64), 0
 ; CHECK: [[ARG1:%[0-9]+]](s192) = G_INSERT [[ARG0]], [[I64]](s64), 64
 ; CHECK: [[ARG2:%[0-9]+]](s192) = G_INSERT [[ARG1]], [[I8]](s8), 128
 ; CHECK: [[ARG:%[0-9]+]](s192) = COPY [[ARG2]]
 
define i64 @test_arr_call([4 x i64]* %addr) {
   %res = call [4 x i64] @arr_callee([4 x i64] %arg)
   %val = extractvalue [4 x i64] %res, 1
   ret i64 %val
 }
 
 
 ; CHECK-LABEL: name: test_abi_exts_call
 ; CHECK: [[VAL:%[0-9]+]](s8) = G_LOAD
-; CHECK: %w0 = COPY [[VAL]]
+; CHECK: [[VAL_TMP:%[0-9]+]](s32) = G_ANYEXT [[VAL]]
+; CHECK: %w0 = COPY [[VAL_TMP]]
 ; CHECK: BL @take_char, csr_aarch64_aapcs, implicit-def %lr, implicit %sp, implicit %w0
 ; CHECK: [[SVAL:%[0-9]+]](s32) = G_SEXT [[VAL]](s8)
 ; CHECK: %w0 = COPY [[SVAL]](s32)
 ; CHECK: BL @take_char, csr_aarch64_aapcs, implicit-def %lr, implicit %sp, implicit %w0
 ; CHECK: [[ZVAL:%[0-9]+]](s32) = G_ZEXT [[VAL]](s8)
 ; CHECK: %w0 = COPY [[ZVAL]](s32)
 ; CHECK: BL @take_char, csr_aarch64_aapcs, implicit-def %lr, implicit %sp, implicit %w0
 declare void @take_char(i8)

test/Verifier/test_copy.mir

+#RUN: not llc -o - -global-isel -run-pass=none -verify-machineinstrs %s 2>&1 | FileCheck %s
+# REQUIRES: global-isel, aarch64-registered-target
+--- |
+  ; ModuleID = 'test.ll'
+  source_filename = "test.ll"
+  target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+  target triple = "aarch64-unknown-unknown"
+  
+  define i32 @test_copy(i32 %argc) {
+    ret i32 0
+  }
+
+...
+---
+name:            test_copy
+legalized:       true
+regBankSelected: false
+selected:        false
+tracksRegLiveness: true
+registers:       
+  - { id: 0, class: _, preferred-register: '' }
+liveins:         
+body:             |
+  bb.0:
+    liveins: %w0
+    ; This test is used to catch verifier errors with copys having mismatching sizes
+    ; CHECK: Bad machine code: Copy Instruction is illegal with mismatching sizes
+  
+    %0(s8) = COPY %w0
+...