[PATCH] Fix MachineCopyPropagation with subregs

[James Molloy]

Thanks Jakob,

Committed in r199797.

From: Jakob Stoklund Olesen [mailto:stoklund at 2pi.dk]
Sent: 21 January 2014 19:19
To: James Molloy
Cc: Commit Messages and Patches for LLVM; Tim Northover
Subject: Re: [PATCH] Fix MachineCopyPropagation with subregs


On Jan 21, 2014, at 10:30 AM, James Molloy <James.Molloy at arm.com<mailto:James.Molloy at arm.com>> wrote:


Hi Tim (and other reviewers),

MachineCopyPropagation has special logic for removing COPY instructions. It will remove plain COPYs using eraseFromParent(), but if the COPY has imp-defs/imp-uses it will convert it to a KILL, to keep the imp-def around.

This actually totally breaks and causes the machine verifier to cry in several cases, one of which being:

%RAX<def> = COPY %RCX<kill>
%ECX<def> = COPY %EAX<kill>, %RAX<imp-use,kill>

These subregister copies are together identified as noops, so are both removed. However, the second one as it has an imp-use gets converted into a kill:

%ECX<def> = KILL %EAX<kill>, %RAX<imp-use,kill>

As the original COPY has been removed, the verifier goes into tears at the use of undefined EAX and RAX.

These implicit register operands after register allocation are quite annoying and really hard to get right in a pass like this. They do provide a more precise liveness tracking, but I don't think that a lot of targets actually benefit from the extra precision.

I think we should consider moving to a model where a register use is valid if any part the the register is live, instead of the current model where the full register is required to be live before a use. AFAICT, this would allow us to get rid of all those implicit operands.

PEI / RegScavenger and the PostRA scheduler are the main users of the late liveness tracking. That's where the regressions would be.

The patch looks good to me unless Tim has comments.

Thanks,
/jakob


There are several hacky solutions to this hacky problem (which is all to do with imp-use/def weirdnesses), but the least hacky I've come up with is to *always* remove COPYs by converting to KILLs. KILLs are no-ops to the code generator so the generated code doesn't change (which is why they were partially used in the first place), but using them also keeps the def/use and imp-def/imp-use chains alive:

%RAX<def> = KILL %RCX<kill>
%ECX<def> = KILL %EAX<kill>, %RAX<imp-use,kill>

The patch passes all test cases including the ones that check the removal of MOVs in this circumstance, along with an extra test I added to check subregister behaviour (which made the machine verifier fall over before my patch).

The patch also adds some DEBUG() statements because the file hadn't got any.

Please review!

Cheers,

James

<mcp-removecopy.diff>_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits


-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140122/60311d61/attachment.html>