[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.
----------------
aditya_nandakumar wrote:
> 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).
That would be perfect.


Repository:
  rL LLVM

https://reviews.llvm.org/D37775