llvm dev - Jun 2020 - [ARM] Thumb code-gen for 8-bit imm arguments results in extra reg copies

On Tue, 16 Jun 2020 at 15:47, Tim Northover <t.p.northover at gmail.com>
wrote:
>
> On Tue, 16 Jun 2020 at 10:23, Prathamesh Kulkarni via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> > (b) Modifies RegisterCoalescer::reMaterializeTrivialDef and
> > TargetInstrInfo::isReallyTriviallyReMaterializableGeneric to check
> > for single live def, instead of single def.
>
> This seems dodgy to me. The instruction does also change CPSR so for
> the transformation to be valid you have to know that register is dead
> where the new instruction is being inserted. As far as I can tell the
> hasOneDef check in reMaterializeTrivialDef is a simple heuristic to
> keep the analysis local and avoid dealing with such issues.
>
> I also don't know how other users of the rematerializable property
> will cope with a wider range of
> isReallyTriviallyReMaterializableGeneric instructions.
>
> Once you start having to consider the surroundings, is it still
> "trivial"? I honestly don't know right now.
Hi Tim,
Thanks for the response!
Sorry, I didn't entirely understand why it isn't safe to propagate
while checking for single live def, could you please elaborate ?
IIUC, for above case, tmovi8 (movs) would update cpsr n/z flags, but
since it's overwritten by following instructions, it's marked as dead
in both cases ?
The patch just checks that only one def is live and remaining defs of
DefMI are dead in reMaterializeTrivialDef, in which case I assume it
should be safe to
propagate value of CopyMI into DefMI ?
If reMaterializeTrivialDef is not the right place to handle the
transform, could you please suggest where should I look for adding it
?
Thanks!

Regards,
Prathamesh
>
> Cheers.
>
> Tim.

On Thu, 18 Jun 2020 at 10:39, Prathamesh Kulkarni
<prathamesh.kulkarni at linaro.org> wrote:
> Sorry, I didn't entirely understand why it isn't safe to propagate
> while checking for single live def, could you please elaborate?
Imagine something like:

    %1:gpr = tMOVi8 123, def dead %cpsr
    [...]
    tCMPr %2:gpr, %3:gpr, def %cpsr
    %4:gpr = COPY %1:gpr
    tBcc %bb.1, 1, implicit %cpsr

The initial mov only has one live use, but reinserting it between the
cmp and the bne would make the branch go to the wrong place. You need
%cpsr to be dead at the place you're intending to insert the mov too.
> If reMaterializeTrivialDef is not the right place to handle the
> transform, could you please suggest where should I look for adding it?
It might be the right place, otherwise it might fit in with a separate
ARM pass of its own, or a known more expensive computation that hasn't
been written yet. I don't know, but we should at least consider
whether we want to complicate the interface for these functions.

Cheers.

Tim.

On Thu, 18 Jun 2020 at 18:22, Tim Northover <t.p.northover at gmail.com>
wrote:
>
> On Thu, 18 Jun 2020 at 10:39, Prathamesh Kulkarni
> <prathamesh.kulkarni at linaro.org> wrote:
> > Sorry, I didn't entirely understand why it isn't safe to
propagate
> > while checking for single live def, could you please elaborate?
>
> Imagine something like:
>
>     %1:gpr = tMOVi8 123, def dead %cpsr
>     [...]
>     tCMPr %2:gpr, %3:gpr, def %cpsr
>     %4:gpr = COPY %1:gpr
>     tBcc %bb.1, 1, implicit %cpsr
>
> The initial mov only has one live use, but reinserting it between the
> cmp and the bne would make the branch go to the wrong place. You need
> %cpsr to be dead at the place you're intending to insert the mov too.
Hi Tim,
Ah indeed, thanks for the clarification!
Would it make sense to restrict the transform where:
(a) DefMI and CopyMI are in same basic block
(b) Check that at least one of the instructions following DefMI till
end of block define cpsr
before it is used ? So the value of cpsr at DefMI is still dead.

Is there any API I could use to check if a particular instruction
defines / uses cpsr ?

Thanks,
Prathamesh
>
> > If reMaterializeTrivialDef is not the right place to handle the
> > transform, could you please suggest where should I look for adding it?
>
> It might be the right place, otherwise it might fit in with a separate
> ARM pass of its own, or a known more expensive computation that hasn't
> been written yet. I don't know, but we should at least consider
> whether we want to complicate the interface for these functions.
>
> Cheers.
>
> Tim.