[PATCH] Fix MachineCopyPropagation with subregs

[Hal Finkel]

----- Original Message -----
> From: "Jakob Stoklund Olesen" <stoklund at 2pi.dk>
> To: "James Molloy" <James.Molloy at arm.com>
> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
> Sent: Tuesday, January 21, 2014 1:18:30 PM
> Subject: Re: [PATCH] Fix MachineCopyPropagation with subregs
> 
> 
> 
> 
> 
> On Jan 21, 2014, at 10:30 AM, James Molloy < 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.

Am I correct in thinking that the benefit comes on targets that have independently-usable subregisters (so that, for example, the scavenger might separately allocate both al and ah on x86)? Maybe ARM would notice if we always "upcasted" liveness to the largest super-register (since they have those register pairs, etc.).

 -Hal

> 
> 
> 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
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory