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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet accepted this revision.
qcolombet added a comment.
This revision is now accepted and ready to land.

LGTM with couple of nits below.



================
Comment at: lib/Target/X86/X86CallLowering.cpp:258
+      // case ValVT == LocVT == MVT::f32. If LocSize and ValSize are equal
+      // we expect this to be handled in SExt/ZExt/AExt case.
+      unsigned PhysRegSize =
----------------
I would have expected the opposite in the comment:
If LocSize and ValSize are *not* equal


================
Comment at: lib/Target/X86/X86InstructionSelector.cpp:799
+    return true;
+  }
+
----------------
qcolombet wrote:
> 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.
I believe we could share some more code:
- Turn this whole code sequence as selectTurnIntoCopy (or something) (BTW the refactoring for canTurnIntoCOPY looks good).
- The expansion in trunc would simply turn into a call to that method
- The expansion in any_ext would do a the same with the swap argument


================
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);
 
----------------
aivchenk wrote:
> qcolombet wrote:
> > When is this pattern created?
> 
> It can be seen in regbankselect-X86_64.mir:
> 
>   define float @test_undef3() {
>     ret float undef
>   }
> 
> before legalizer:
> 
>     %1(s128) = G_IMPLICIT_DEF
>     %xmm0 = COPY %1(s128)
>     RET 0, implicit %xmm0
> 
> after legalizer:
> 
>     %1:_(s128) = G_IMPLICIT_DEF
>     %xmm0 = COPY %1(s128)
>     RET 0, implicit %xmm0
> 
> 
Got it, however, that could be a separate patch with its own test, right?


https://reviews.llvm.org/D42287