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

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar 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.
----------------
qcolombet wrote:
> 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).
I can update the check to do it only when there are generic types involved (ie one of src and dst has a generic virtual register).


Repository:
  rL LLVM

https://reviews.llvm.org/D37775