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

via llvm-commits llvm-commits at lists.llvm.org
Tue Feb 27 09:03:46 PST 2018


This change has been re-enabled in r326208, now that the renamable flag setting has been made more conservative by default and is opt-in on a per-target basis so as not to effect out-of-tree targets. Please let me know if there any more issues.

 

-- 

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: qcolombet at apple.com [mailto:qcolombet at apple.com] 
Sent: Friday, February 16, 2018 10:10 PM
To: gberry at codeaurora.org
Cc: escha at apple.com; Nemanja Ivanovic <nemanja.i.ibm at gmail.com>; junbuml at codeaurora.org; marina.yatsina at intel.com; reviews+D41835+public+9c1dec7fb6e75ce0 at reviews.llvm.org; Nicolai Hähnle-Montoro <nhaehnle at gmail.com>; Matthias Braun <matze at braunis.de>; llvm-commits <llvm-commits at lists.llvm.org>; tpr.llvm at botech.co.uk; javed.absar at arm.com
Subject: Re: [PATCH] D41835: [MachineCopyPropagation] Extend pass to do COPY source forwarding

 

FYI, I've revert the commit in r325421. It was getting messing :)





On Feb 7, 2018, at 1:38 PM, gberry at codeaurora.org <mailto:gberry at codeaurora.org>  wrote:

 

I’ve posted a patch that I believe addresses everyone’s concerns here:  <https://reviews.llvm.org/D43042> https://reviews.llvm.org/D43042

Please take a look.

 

-- 

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] On Behalf Of escha at apple.com <mailto:escha at apple.com> 
Sent: Wednesday, February 7, 2018 12:52 PM
To: gberry at codeaurora.org <mailto:gberry at codeaurora.org> 
Cc: qcolombet at apple.com <mailto:qcolombet at apple.com> ; Nemanja Ivanovic <nemanja.i.ibm at gmail.com <mailto:nemanja.i.ibm at gmail.com> >; junbuml at codeaurora.org <mailto:junbuml at codeaurora.org> ; marina.yatsina at intel.com <mailto:marina.yatsina at intel.com> ; reviews+D41835+public+9c1dec7fb6e75ce0 at reviews.llvm.org <mailto:reviews+D41835+public+9c1dec7fb6e75ce0 at reviews.llvm.org> ; Nicolai Hähnle-Montoro <nhaehnle at gmail.com <mailto:nhaehnle at gmail.com> >; Matthias Braun <matze at braunis.de <mailto:matze at braunis.de> >; 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> ; javed.absar at arm.com <mailto:javed.absar at arm.com> 
Subject: Re: [PATCH] D41835: [MachineCopyPropagation] Extend pass to do COPY source forwarding

 

 






On Feb 5, 2018, at 11:27 AM, via llvm-commits < <mailto:llvm-commits at lists.llvm.org> llvm-commits at lists.llvm.org> wrote:

 

 

From:  <mailto:qcolombet at apple.com> qcolombet at apple.com [ <mailto:qcolombet at apple.com> mailto:qcolombet at apple.com] 
Sent: Friday, February 2, 2018 8:18 PM





On Feb 2, 2018, at 3:53 PM, Nemanja Ivanovic via llvm-commits < <mailto:llvm-commits at lists.llvm.org> 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.

 

 

Ok, I will put together a strawman proposal for adding this target global allowRegisterRenaming flag.

 

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.

 

 

what’s the status of this; are we planning on reverting this in llvm for now? i ask because currently i’m handling the problem in our tree by keeping this patch reverted, but we really try to avoid long-term divergence from upstream llvm both due to the hassles it causes us and the difficulties it causes in trying to contribute patches back to upstream llvm.

 

thanks for looking into this!

 

—escha

 

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


More information about the llvm-commits mailing list