[LLVMdev] ScheduleDAGInstrs/R600 test potential issue with implicit defs

[Tom Stellard]

On Tue, Feb 25, 2014 at 07:22:54PM +0100, Stefan Hepp wrote:
> Hi Tom,
> 
> Thanks a lot for your explanations, now it makes a lot more sense ;)
> 
> I had a slightly closer look at the R600 packetizer, and the issue is 
> that the third LSHL instruction has both an implicit use and 
> *afterwards* an implicit def of T1_XYZW. The latter def causes the 
> current ScheduleDAGInstrs implementation to ignore the implicit use, 
> thus the ScheduleDAG only contains an anti-dependency from the second to 
> the third LSHL and the packetizer can bundle the instructions.
> 
> If the order of the implicit-defs and implicit-use would be different 
> (e.g., like TableGen adds them), or if I apply my patch so that the 
> implicit-use is not ignored, there is a true dependency edge between 
> those two instructions, preventing bundling.
> 
> Removing only the implicit-defs alone has no effect in this example, as 
> then the implicit-use of T1_XYZW is then no longer removed and a true 
> dependency to the def of T1_W is added. Removing all implicit operands 
> however solves that (see attached patch/hack).
> 
> I would argue that the behaviour of ScheduleDAGInstrs::buildSchedGraph 
> for implicit operands is not correct, simply because changing the order 
> of the operands would already prevent the R600 packetizer from bundling 
> in this example.
>

It does sound like a bug.  You might want to ping the scheduling code
owner, Andrew Trick.


> Cheers,
>   Stefan
> 

> From 451410290c9c1ad60d5b827e930ef949078ec6c6 Mon Sep 17 00:00:00 2001
> From: Stefan Hepp <stefan at stefant.org>
> Date: Tue, 25 Feb 2014 19:08:58 +0100
> Subject: [PATCH] Remove implicit defs and uses before packetizing in R600
>  backend to avoid dependencies between super-registers
> 
> ---
>  lib/Target/R600/R600Packetizer.cpp | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/lib/Target/R600/R600Packetizer.cpp b/lib/Target/R600/R600Packetizer.cpp
> index cd9b6ea..8754bbb1 100644
> --- a/lib/Target/R600/R600Packetizer.cpp
> +++ b/lib/Target/R600/R600Packetizer.cpp
> @@ -356,6 +356,15 @@ bool R600Packetizer::runOnMachineFunction(MachineFunction &Fn) {
>          End = MBB->end();
>          continue;
>        }
> +      for (size_t idx = MI->getNumExplicitOperands();
> +           idx < MI->getNumOperands(); idx++)
> +      {
> +        MachineOperand &MO = MI->getOperand(idx);
> +        if (MO.isImplicit()) {

This should be:

           if (MO.isImplicit() &&
               AMDGPU::R600_Reg128.contains(MO.getReg())) {


That way it will only remove super-register defs and uses.

-Tom

> +          MI->RemoveOperand(idx);
> +          idx--;
> +        }
> +      }
>        ++MI;
>      }
>    }
> -- 
> 1.8.3.2
>