[PATCH] D22420: Fix check for prescheduling to copies from virtual registers

[Justin Bogner via llvm-commits]

"Elliot Colp" <colpell at ca.ibm.com> writes:
> Hmm, you're right -- seems like I misread that line. Looking at this with
> fresh eyes, I agree that this change doesn't look correct.
>
> This patch was an attempt to fix an error related to DAG scheduling when
> compiling for SystemZ, but evidently this isn't the right way to do it.
> Who would be best to talk to about this part of the code so I can get a
> better idea of what's going wrong?

That's a tricky question in the abstract ;) - The #llvm channel on
irc.oftc.net is a good place to ask questions, or here on the lists.

I would recommend filing a bug (llvm.org/bugs) with an example of the
errors you're seeing - this will get a few more eyes on it and provide a
nice place to refer to when discussing the problem. 

> Justin Bogner <justin at justinbogner.com> wrote on 2016/07/16 02:54:24 PM:
>
>> From: Justin Bogner <mail at justinbogner.com>
>> To: Elliot Colp/Toronto/IBM at IBMCA
>> Cc: reviews+D22420+public+c33f2e5d5d477fac at reviews.llvm.org,
>> matze at braunis.de, llvm-commits at lists.llvm.org
>> Date: 2016/07/16 02:54 PM
>> Subject: Re: [PATCH] D22420: Fix check for prescheduling to copies
>> from virtual registers
>> Sent by: Justin Bogner <justin at justinbogner.com>
>>
>> Elliot Colp <colpell at ca.ibm.com> writes:
>> > colpell created this revision.
>> > colpell added a reviewer: bogner.
>> > colpell added a subscriber: llvm-commits.
>> > Herald added a subscriber: MatzeB.
>> >
>> > Currently, this check on line 2834 is an exact copy of a check on line
>> > 2810, which SU should have already passed.
>>
>> That's not entirely true. They both check SU.getNode(), sure, but one
>> checks if the Opcode is CopyToReg and the other if it's CopyFromReg.
>> This change definitely changes behaviour and needs to come with a test
>> if it's correct, which I'm not really convinced it is.
>>
>> > Given the context and comment above the check ("Avoid prescheduling
>> > *to* copies" vs. "Avoid prescheduling copies"), it seems like the
>> > intention was to check PredSU instead.
>>
>> > https://reviews.llvm.org/D22420
>> >
>> > Files:
>> >   lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
>> >
>> > Index: lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
>> > ===================================================================
>> > --- lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
>> > +++ lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
>> > @@ -2831,7 +2831,7 @@
>> >        continue;
>> >      // Avoid prescheduling to copies from virtual registers,
>> which don't behave
>> >      // like other nodes from the perspective of scheduling heuristics.
>> > -    if (SDNode *N = SU.getNode())
>> > +    if (SDNode *N = PredSU->getNode())
>> >        if (N->getOpcode() == ISD::CopyFromReg &&
>> >            TargetRegisterInfo::isVirtualRegister
>> >              (cast<RegisterSDNode>(N->getOperand(1))->getReg()))
>> >
>>