Add a verifier test to check the access on both sides of COPY are the same
AcceptedPublic

Authored by aditya_nandakumar on Sep 12 2017, 3:32 PM.

Details

Summary

Only checks if there are no sub registers involved for now.
There are lots of failures currently which need to be fixed by various backends.

Splitting this off from https://reviews.llvm.org/D37640

Diff Detail

Failed to build as a shared library -DBUILD_SHARED_LIBS=1

igorb added inline comments.Sep 13 2017, 1:17 AM
lib/CodeGen/MachineVerifier.cpp
982

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

qcolombet added inline comments.Sep 13 2017, 9:36 AM
lib/CodeGen/MachineVerifier.cpp
982

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).

lib/CodeGen/MachineVerifier.cpp
982

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).

qcolombet added inline comments.Sep 13 2017, 10:42 AM
lib/CodeGen/MachineVerifier.cpp
982

That would be perfect.

Updated to check only if one of Src/Dst has generic virtual registers.

aditya_nandakumar marked an inline comment as done.
aditya_nandakumar edited the summary of this revision. (Show Details)

Updated to refactor getRegSizeInBits into TargetRegisterInfo. There are 57 failures in total.

qcolombet accepted this revision.Sep 15 2017, 2:28 PM

LGTM with one suggestion

lib/CodeGen/MachineVerifier.cpp
986

Seeing this pattern being actually the implementation of RegisterBankInfo::getSizeInBits, I would suggest to move the whole RegisterBankInfo::getSizeInBits directly in TargetRegisterInfo.

This revision is now accepted and ready to land.Sep 15 2017, 2:28 PM

Also, there are various tests that need to be updated by the various backends, and I can only land once those are updated. I haven't had time to update all the backends.

lib/CodeGen/MachineVerifier.cpp
986

I thought TargetRegisterInfo doesn't have LLT headers pulled in (but looks like it is getting transitively pulled in). I can pretty much move the implementation of RegisterBankInfo::getSizeInBits into TargetRegisterInfo.

moved getSizeInBits to TargetRegisterInfo as suggested by Quentin.

qcolombet accepted this revision.Sep 18 2017, 10:51 AM

Updated AArch64 tests.

Would it be possible for someone to update the ARM/X86 tests?

rovka added a comment.Oct 17 2017, 1:56 AM

Sorry, I just noticed this. I'll have a look at the ARM side of things.

rovka added a comment.Oct 19 2017, 4:13 AM

Hi, here's the patch for the ARM GlobalISel tests: https://reviews.llvm.org/differential/diff/119563/
Let me know if there's anything else that needs addressing on ARM.

Thanks Diana.
@igorb would it be possible for you to look at X86 when you get a chance? If not, I can update it.

zvi added a subscriber: zvi.Sun, Oct 29, 2:09 AM

Hi All,

I will try to cover for Igor in helping with the X86 part of this patch.

AFAIU, the challenge is in getting the call/return lowering working for the case where a function's argument/return-value is a f32 or a f64. Some subtargets will use the FP stack registers (which are 80-bit wide), and other will use XMM's (which are 128-bit wide).
Example:

define float @foo(float %arg1, float %arg2) {
  ret float %arg2
}

Existing state on ToT: llc -mtriple=x86_64-linux-gnu -mattr=+sse2 -global-isel -stop-after=irtranslator

body:             |
  bb.1 (%ir-block.0):
    liveins: %xmm0, %xmm1

    %0:_(s32) = COPY %xmm0
    %1:_(s32) = COPY %xmm1
    %xmm0 = COPY %1(s32)
    RET 0, implicit %xmm0

Here are some options for resolving this issue:

  1. Perform the full expansion in X86CallLowering using G_COPY/G_TRUNC and G_ANYEXT/G_COPY. Fix whatever needs fixing to make it work.
  2. X86CallLoweing will continue with generation of G_COPY's with incompatible src/dest bit-sizes, but will assign register classes to the virtual registers. This will technically work because the verification check is guarded by the condition 'SrcTy.isValid() || DstTy.isValid()', which will not be met if final register classes are assigned.
  3. Introduce an X86-specific COPY instruction which will be pattern-matched in Instruction-Selection. This instruction will be generated instead of G_COPY's.

I haven't tried implementing any of the above options, and given my limited knowledge in GlobalISel, would appreciate any feedback on this.

Thanks, Zvi

I would definitely prefer the sizes to be right coming out of the CallLowering - so option #1.