Page MenuHomePhabricator

[GISel]: Fix generation of illegal COPYs (of different sizes) during CallLowering
ClosedPublic

Authored by aditya_nandakumar on Sep 8 2017, 1:05 PM.

Details

Summary

We end up creating COPY's that are either truncating/extending and this should be illegal.
Eg %0(s8) = COPY %w0
Instead, we generate TRUNCs/ANY_EXT and let the optimizer deal with it later.
%tmp(s32) = COPY %w0
%0(s8) = G_TRUNC %tmp(s32)

I've updated the AArch64 backend, I'd appreciate it if people more familiar with X86/ARM can update it as well(post feedback).

Looking forward to your comments.

Diff Detail

Event Timeline

igorb edited edge metadata.Sep 11 2017, 3:15 AM

X86 backend part - https://reviews.llvm.org/D37678
Could you please merge into you patch.
Regards.

qcolombet edited edge metadata.Sep 11 2017, 12:14 PM

Hi Aditya,

Could you add a check in the verifier too?

Cheers,
-Quentin

Added verifier check for catching simple cases (non subreg indices for now).

Adding the verifier check has resulted in 5, 12, and 43 failures in ARM, AArch64, and X86 respectively which needs to be fixed.

The AArch64 part looks good to me.

include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
549

You can split the patch for those, when you commit.

670

Ditto.

lib/CodeGen/MachineVerifier.cpp
52

This change is problematic because we add a dependency from CodeGen to GlobalISel

953

Using the type of the vregs, MachineRegisterInfo::getType, should eliminate the dependency on RegisterBankInfo.

aditya_nandakumar marked 2 inline comments as done.Sep 13 2017, 11:27 AM
aditya_nandakumar added inline comments.
lib/CodeGen/MachineVerifier.cpp
953

This part has moved to https://reviews.llvm.org/D37775
MRI->getType would only give me valid types for generic virtual registers.
For cases such as %0(s16) = COPY %W0, I'd still need to get the size of W0. If dependency is the issue, I can certainly do what RegisterBankInfo::getSizeInBits() is doing, but that's duplicating work.

qcolombet added inline comments.Sep 13 2017, 2:56 PM
lib/CodeGen/MachineVerifier.cpp
953

I would push the interesting part of getSizeInBits from RegisterBankInfo to TargetRegisterInfo and have RegisterBankInfo::getSizeInBits uses TargetRegisterInfo::getSizeInBits to avoid the duplication while having the necessary functionality available for the verifier.

aditya_nandakumar marked an inline comment as done.Sep 14 2017, 2:43 PM
aditya_nandakumar added inline comments.
lib/CodeGen/MachineVerifier.cpp
953

Quentin - I refactored getSizeInBits in the other review.

aditya_nandakumar marked an inline comment as done.

Merged X86 part from https://reviews.llvm.org/D37678
Thanks Igor.

Updated ARM-irtranslator.ll test to account for extendRegister emitting G_ANYEXT.

Now, all of the backends pass tests - but LowerFormalArguments still generates wrong (truncating) copies. This will likely be an issue when https://reviews.llvm.org/D37775 lands
Could someone familiar with ARM codebase implement that?

rovka edited edge metadata.Oct 3 2017, 3:34 AM

Could someone familiar with ARM codebase implement that?

I'll look into it.

rovka added a comment.Oct 6 2017, 11:06 AM

Hi Aditya,

I uploaded the ARM changes here: https://reviews.llvm.org/differential/diff/118037/

As a sidenote, this caused trouble with some of my integration tests because now the legalize combiner finds all sorts of opportunities to replace trunc + ext with shifts, which weren't implemented on ARM yet (they are now). I don't think the combiner should be introducing instructions without first checking if they are legal, maybe you can consider fixing that in the future.

Thanks for the patch Diana.
Also regarding the side node, I'll be looking into that soon.

Updated with Diana's patch for ARM.

Can I get an LGTM for this if there are no other comments/concerns?

This revision is now accepted and ready to land.Oct 9 2017, 11:50 AM

Committed in r315240