[PATCH] D41835: [MachineCopyPropagation] Extend pass to do COPY source forwarding

Fri Feb 2 17:18:29 PST 2018

> On Feb 2, 2018, at 3:53 PM, Nemanja Ivanovic via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> Sorry, but it seems to me that we are working around the problem with the hasExtraRegAllocReq flag and this workaround is introducing new problems that then need to be solved.
> Going back to the original problem that Escha reported, it seems to be that the back end ends up in the following state which arguably is probably not a reasonable state to be in (if register renaming is to be allowed):
> 
> - There are constraints added to the virtual registers that restrict which register class the assigned physical register must come from
> - The register allocator respects those constraints and selects a register from the correct class
> - The selected physical register is marked as renamable
> - Calling MachineInstr::getRegClassConstraint() on the machine instruction returns a register class that does not take into account the additional constraints
> 
> Ultimately, anything that does register renaming with the back end in that state can potentially break the code since it doesn't know what actual class the register rename is allowed to come from.
> 
> Wouldn't this mean that a target that can be in this state has at least these two reasonable options:
> 1. Set some property that will ensure that the renamable flag is never set on any register - essentially what Geoff is suggesting in the latter part of his last email. I would just make it a property of TargetRegisterInfo (perhaps `virtual bool TargetRegisterInfo::allowRegisterRenaming()`).

Again, yes I think we need that.

More generally, I believe we want to step back with the renamable flag, think about its semantic and what in the infrastructure we would need to fix, if any, to make sure that semantic is preserved/preservable along the way.

> 2. We make TargetInstrInfo::getRegClass() virtual and allow the target to override it so that the extra constraints can be taken into account if it wants to allow renaming of registers
> 
> Not to suggest that the two solutions are mutually exclusive.
> 
> On Fri, Feb 2, 2018 at 5:57 PM, <gberry at codeaurora.org <mailto:gberry at codeaurora.org>> wrote:
> I think the fix for this the way things are currently setup would be for the code that changes the COPY to the target opcode to clear the renamable bits on the operands.
> 

Although it would work, I think it hints that we didn’t think the handle of the renamable flag all the way. I feel like we are reproducing the same problems that the kill flag had whereas we are trying to kill the kill flag.
Let us no repeat history :).

> If we add a target global “all opcodes are hasExtraRegAllocReq” setting, we could avoid setting any COPY operands as renamable in the first place.
> 
>  
> 
> --
> 
> Geoff Berry
> 
> Employee of Qualcomm Datacenter Technologies, Inc.
> 
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
> 
>  
> 
> From: fglaser at apple.com <mailto:fglaser at apple.com> [mailto:fglaser at apple.com <mailto:fglaser at apple.com>] On Behalf Of escha at apple.com <mailto:escha at apple.com>
> Sent: Friday, February 2, 2018 5:30 PM
> To: escha at apple.com <mailto:escha at apple.com>
> Cc: gberry at codeaurora.org <mailto:gberry at codeaurora.org>; junbuml at codeaurora.org <mailto:junbuml at codeaurora.org>; marina.yatsina at intel.com <mailto:marina.yatsina at intel.com>; kannan.narayanan at amd.com <mailto:kannan.narayanan at amd.com>; nhaehnle at gmail.com <mailto:nhaehnle at gmail.com>; wei.ding2 at amd.com <mailto:wei.ding2 at amd.com>; Matthias Braun <matze at braunis.de <mailto:matze at braunis.de>>; Nemanja Ivanovic <nemanja.i.ibm at gmail.com <mailto:nemanja.i.ibm at gmail.com>>; llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>>; reviews+D41835+public+9c1dec7fb6e75ce0 at reviews.llvm.org <mailto:reviews%2BD41835%2Bpublic%2B9c1dec7fb6e75ce0 at reviews.llvm.org>; tpr.llvm at botech.co.uk <mailto:tpr.llvm at botech.co.uk>; javed.absar at arm.com <mailto:javed.absar at arm.com>
> 
> Subject: Re: [PATCH] D41835: [MachineCopyPropagation] Extend pass to do COPY source forwarding
> 
>  
> 
> update: it gets worse. i can’t get the tests to pass because a lot of tests fail with this when the verifier is on:
> 
>  
> *** Bad machine code: Illegal isRenamable setting for opcode with extra regalloc requirements ***
>  
> 
> This occurs on our copy instructions after we expand them into actual machine opcodes, because the machine opcode for the copy instruction isn’t Renamable (none of our instructions are).
> 
>  
> 
> —escha
> 
>  
> 
> 
> 
> 
> On Feb 2, 2018, at 1:37 PM, via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
> 
>  
> 
> this is probably a reasonable solution. mostly, at this point, i’m worried about the fact that upstream now apparently has a different understanding of register classes than we do (in terms of what is “safe”), which could lead to other future issues for us.
> 
>  
> 
> —escha
> 
> 
> 
> 
> On Feb 2, 2018, at 1:36 PM, gberry at codeaurora.org <mailto:gberry at codeaurora.org> wrote:
> 
>  
> 
> Ah, okay.  So I assume you’re okay with disabling the post-RA run of this pass for this particular target to fix this particular issue?
> 
>  
> 
> -- 
> 
> Geoff Berry
> 
> Employee of Qualcomm Datacenter Technologies, Inc.
> 
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
> 
>  
> 
> From: fglaser at apple.com <mailto:fglaser at apple.com> [mailto:fglaser at apple.com <mailto:fglaser at apple.com>] On Behalf Of escha at apple.com <mailto:escha at apple.com>
> Sent: Friday, February 2, 2018 4:32 PM
> To: gberry at codeaurora.org <mailto:gberry at codeaurora.org>
> Cc: Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>>; reviews+D41835+public+9c1dec7fb6e75ce0 at reviews.llvm.org <mailto:reviews+D41835+public+9c1dec7fb6e75ce0 at reviews.llvm.org>; Geoff Berry via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>>; javed.absar at arm.com <mailto:javed.absar at arm.com>; Matthias Braun <matze at braunis.de <mailto:matze at braunis.de>>; Jonas Paulsson <paulsson at linux.vnet.ibm.com <mailto:paulsson at linux.vnet.ibm.com>>; tstellar at redhat.com <mailto:tstellar at redhat.com>; Matt Arsenault <Matthew.Arsenault at amd.com <mailto:Matthew.Arsenault at amd.com>>; junbuml at codeaurora.org <mailto:junbuml at codeaurora.org>; marina.yatsina at intel.com <mailto:marina.yatsina at intel.com>; wei.ding2 at amd.com <mailto:wei.ding2 at amd.com>; kannan.narayanan at amd.com <mailto:kannan.narayanan at amd.com>; nhaehnle at gmail.com <mailto:nhaehnle at gmail.com>; Nemanja Ivanovic <nemanja.i.ibm at gmail.com <mailto:nemanja.i.ibm at gmail.com>>; llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>>; tpr.llvm at botech.co.uk <mailto:tpr.llvm at botech.co.uk>
> Subject: Re: [PATCH] D41835: [MachineCopyPropagation] Extend pass to do COPY source forwarding
> 
>  
> 
> looking at it again, it’s making more sense: your patch enabled MachineCopyPropagation in the RA pipeline. the pass might have *already* been broken, but it wasn’t running for us, and now it is, so it broke things in that way too.
> 
>  
> 
> —escha
> 
> 
> 
> 
> 
> On Feb 2, 2018, at 1:26 PM, gberry at codeaurora.org <mailto:gberry at codeaurora.org> wrote:
> 
>  
> 
> escha,
> 
>  
> 
> I’m confused by your comment about bundles.  It was known that this patch doesn’t handle forwarding into bundles, but it sounds like to me you are seeing problems with the dead COPY removal part of this pass, which was not intentionally changed by this patch (other than adding a DEBUG statement when it happens).
> 
>  
> 
> -- 
> 
> Geoff Berry
> 
> Employee of Qualcomm Datacenter Technologies, Inc.
> 
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
> 
>  
> 
> From: fglaser at apple.com <mailto:fglaser at apple.com> [mailto:fglaser at apple.com <mailto:fglaser at apple.com>] On Behalf Of escha at apple.com <mailto:escha at apple.com>
> Sent: Friday, February 2, 2018 4:10 PM
> To: Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>>
> Cc: Geoff Berry <gberry at codeaurora.org <mailto:gberry at codeaurora.org>>; reviews+D41835+public+9c1dec7fb6e75ce0 at reviews.llvm.org <mailto:reviews+D41835+public+9c1dec7fb6e75ce0 at reviews.llvm.org>; Geoff Berry via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>>; javed.absar at arm.com <mailto:javed.absar at arm.com>; Matthias Braun <matze at braunis.de <mailto:matze at braunis.de>>; Jonas Paulsson <paulsson at linux.vnet.ibm.com <mailto:paulsson at linux.vnet.ibm.com>>; tstellar at redhat.com <mailto:tstellar at redhat.com>; Matt Arsenault <Matthew.Arsenault at amd.com <mailto:Matthew.Arsenault at amd.com>>; junbuml at codeaurora.org <mailto:junbuml at codeaurora.org>; marina.yatsina at intel.com <mailto:marina.yatsina at intel.com>; wei.ding2 at amd.com <mailto:wei.ding2 at amd.com>; kannan.narayanan at amd.com <mailto:kannan.narayanan at amd.com>; nhaehnle at gmail.com <mailto:nhaehnle at gmail.com>; Nemanja Ivanovic <nemanja.i.ibm at gmail.com <mailto:nemanja.i.ibm at gmail.com>>; llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>>; tpr.llvm at botech.co.uk <mailto:tpr.llvm at botech.co.uk>
> Subject: Re: [PATCH] D41835: [MachineCopyPropagation] Extend pass to do COPY source forwarding
> 
>  
> 
>  
> 
> 
> 
> 
> 
> 
> On Feb 2, 2018, at 12:55 PM, Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>> wrote:
> 
>  
> 
>  
> 
> 
> 
> 
> 
> 
> On Feb 2, 2018, at 12:21 PM, escha at apple.com <mailto:escha at apple.com> wrote:
> 
>  
> 
> I mean, in that case we are likely to have to mark every single opcode (all 12,000 or so) with this requirement. At that point we might as well just opt out of the pass, I think? At least, it feels like a gross hack that papers over the fact that LLVM has changed how register classes work such that our entire approach is no longer valid.
> 
>  
> 
> Also, it seems very weird to make this constraint-violating behavior *opt-out*. Maybe it should be opt-in, i.e. put doesNotHaveExtraSrcRegAllocReq on all instructions it’s okay for?
> 
>  
> 
> That sounds like a better approach to me.
> 
> After talking with escha, I agree that TableGen is not necessarily expressive enough to model all the constraints that need to be met and I would err on the safe side of being opt-in instead of opt-out.
> 
>  
> 
> this is actually worse than i initially realized. one of our targets is VLIW and is completely broken by this patch, *even if we opt out as described*, because it doesn’t iterate over the operands of a bundle. so for example:
> 
>  
> 
> r3 = COPY foo
> 
> BUNDLE_TYPE_FOO
> 
> <thing that uses r3>
> 
> END
> 
>  
> 
> it believes that r3 is dead because it doesn’t iterate over the bundle operands, so it never sees the use of r3.
> 
>  
> 
> —escha
> 
>  
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>  
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180202/d1b686b5/attachment.html>