[PATCH] D42287: [GlobalISel][X86] Fixing failures after https://reviews.llvm.org/D37775

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi Alexander,

Thanks for looking into this.

General direction looks good. Couple of nits inlined, but most importantly, you should commit the autogenerated changes first to make the tests easier to review.

Thanks,
-Quentin



================
Comment at: lib/Target/X86/X86CallLowering.cpp:140
+      auto MIB = MIRBuilder.buildAnyExt(LLT::scalar(PhysRegSize), ValVReg);
+      MIRBuilder.buildCopy(PhysReg, MIB->getOperand(0).getReg());
+      return;
----------------
Could we refactor the code to avoid two similar call to buildCopy?
E.g.,
unsigned ExtReg;
if (PhysRegSize > ValSize) {
  [...]
  ExtReg = MIB->getOperand(0).getReg();
} else {
  ExtReg = extendRegister(ValVReg, VA);
}
MIRBuilder.buildCopy(PhysReg, ExtReg);


================
Comment at: lib/Target/X86/X86CallLowering.cpp:264
     default:
       MIRBuilder.buildCopy(ValVReg, PhysReg);
       break;
----------------
Shouldn't the added lines be here?


================
Comment at: lib/Target/X86/X86InstructionSelector.cpp:666
+  // If that's truncation of xmm to f32, just replace it with copy,
+  // as we are able to select it as a regular move.
+  if ((DstRC == &X86::FR32RegClass || DstRC == &X86::FR32XRegClass ||
----------------
Mentioning both f32 and truncation in the same sentence is kind of strange to me, because f32 shouldn't use G_TRUNC but FPTRUNC.
Instead, I believe we want to say something like, if the value lives on the vector bank and goes into the floating bank, ...


================
Comment at: lib/Target/X86/X86InstructionSelector.cpp:686
   if (!SrcRC)
     return false;
 
----------------
Should we move these ![...]RC checks up?


================
Comment at: lib/Target/X86/X86InstructionSelector.cpp:799
+    return true;
+  }
+
----------------
We should make a helper function out of these lines and the previous chunk (canTurnIntoCOPY?).
I believe the only difference if swapping DstRC and SrcRC.


================
Comment at: lib/Target/X86/X86LegalizerInfo.cpp:169
+  // s128 = EXTEND (G_IMPLICIT_DEF s32/s64) -> s128 = G_IMPLICIT_DEF
+  setAction({G_IMPLICIT_DEF, s128}, Legal);
 
----------------
When is this pattern created?


================
Comment at: test/CodeGen/X86/GlobalISel/callingconv.ll:3
+; RUN: llc -mtriple=i386-linux-gnu -mattr=+sse2  -global-isel -verify-machineinstrs < %s -o - | FileCheck %s --check-prefix=X86
+; RUN: llc -mtriple=x86_64-linux-gnu             -global-isel -verify-machineinstrs < %s -o - | FileCheck %s --check-prefix=X86_64
 
----------------
Could we keep the change in the name of a prefix a separate patch?
(You can apply it before or after, up to you)


================
Comment at: test/CodeGen/X86/GlobalISel/callingconv.ll:329
+; X86_64-NEXT:    .cfi_offset %rbx, -16
+; X86_64-NEXT:    movzbl (%rdi), %ebx
+; X86_64-NEXT:    movl %ebx, %edi
----------------
Do you know why this got better all of the sudden?


================
Comment at: test/CodeGen/X86/GlobalISel/callingconv.ll:404
+; X86_64-NEXT:    movq %rax, %xmm0
+; X86_64-NEXT:    movb $1, %al
+; X86_64-NEXT:    callq variadic_callee
----------------
Why did the scheduling/RA change?


================
Comment at: test/CodeGen/X86/GlobalISel/irtranslator-callingconv.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 ; RUN: llc -mtriple=i386-linux-gnu   -mattr=+sse2 -global-isel -stop-after=irtranslator < %s -o - | FileCheck %s --check-prefix=ALL --check-prefix=X32
----------------
Could do the autogeneration as a separate patch so that the changes are obvious?


================
Comment at: test/CodeGen/X86/GlobalISel/regbankselect-X86_64.mir:1
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 # RUN: llc -mtriple=x86_64-linux-gnu -global-isel                       -run-pass=regbankselect %s -o - | FileCheck %s --check-prefix=CHECK --check-prefix=FAST
----------------
Ditto


================
Comment at: test/CodeGen/X86/GlobalISel/select-fconstant.mir:1
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 #RUN: llc -mtriple=x86_64-linux-gnu -mattr=+sse2 -global-isel                       -run-pass=instruction-select -verify-machineinstrs %s -o - | FileCheck %s --check-prefix=CHECK64 --check-prefix=CHECK_SMALL --check-prefix=CHECK_SMALL64 --check-prefix=CHECK_NOPIC64
----------------
Ditto


================
Comment at: test/CodeGen/X86/GlobalISel/select-fsub-scalar.mir:1
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 # RUN: llc -mtriple=x86_64-linux-gnu                                  -global-isel -run-pass=instruction-select -verify-machineinstrs %s -o - | FileCheck %s --check-prefix=ALL --check-prefix=NO_AVX512VL --check-prefix=NO_AVX512F --check-prefix=SSE
----------------
Ditto


================
Comment at: test/CodeGen/X86/GlobalISel/select-memop-scalar.mir:1
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 # RUN: llc -mtriple=x86_64-linux-gnu                                  -global-isel -run-pass=instruction-select -verify-machineinstrs %s -o - | FileCheck %s --check-prefix=ALL --check-prefix=NO_AVX512VL --check-prefix=NO_AVX512F --check-prefix=SSE
----------------
Ditto


================
Comment at: test/CodeGen/X86/GlobalISel/x86_64-legalize-GV.mir:2
 # RUN: llc -mtriple=x86_64-linux-gnu -global-isel -run-pass=legalizer %s -o - | FileCheck %s --check-prefix=ALL --check-prefix=X64
-# RUN: llc -mtriple=i386-linux-gnu   -global-isel -run-pass=legalizer %s -o - | FileCheck %s --check-prefix=ALL --check-prefix=X32
 --- |
 
----------------
What is the rational for separating the two variants?


https://reviews.llvm.org/D42287