[PATCH] D37775: Add a verifier test to check the access on both sides of COPY are the same

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: lib/CodeGen/MachineVerifier.cpp:954
+        RegisterBankInfo::getSizeInBits(SrcOp.getReg(), *MRI, *TRI);
+    if (DstSize != SrcSize)
+      // Catch only obvious cases not involving subregs for now.
----------------
igorb wrote:
> For X86  back-end this is not always correct.
> I think in case a few register classes with different size are mapped to the same physical registers, it is impossible to identify when the COPY is illegal without some additional target specific info.
>   def FR32 : RegisterClass<"X86", [f32], 32, (sequence "XMM%u", 0, 15)>;
>   def VR128 : RegisterClass<"X86", [v4f32, v2f64, v16i8, v8i16, v4i32, v2i64], 128, (add FR32)>;
> 
> For example the test fail  DAG/GISEL (with the patch)
> ./bin/llc -O0 -mtriple=x86_64-linux-gnu -verify-machineinstrs -stop-before=legalizer
> ./bin/llc -O0 -mtriple=x86_64-linux-gnu -global-isel -verify-machineinstrs -stop-before=legalizer 
> 
>   define float @test_add_float(float %arg1, float %arg2) {
>     %ret = fadd float %arg1, %arg2
>     ret float %ret
>   }
> 
> for the correct MIR.  (xmm size is 128)
>     %0(s32) = COPY %xmm0
>     %1(s32) = COPY %xmm1
>     %2(s32) = G_FADD %0, %1
>     %xmm0 = COPY %2(s32)
> 
> Thanks
> 
I'd argue that when generic types are on one side of a copy, i.e., this is a pre-isel copy, we want the size to exactly match.
Thus, if the target wants to do truncate and whatnot at this point, it needs to use G_TRUNC, G_ANYEXT, and so on.

That being said, the check is too broad. We need to check that only for pre-isel copy. Regular copies require indeed target knowledge (e.g., eflags = copy gpr is valid on x86).


Repository:
  rL LLVM

https://reviews.llvm.org/D37775