[PATCH] [AArch64] Enhance rematerialization by adding a new API isAsCheapAsAMove in TargetInstroInfo

[Jiangning Liu]

Hi Quentin,

I think the key point is you are treating isAsCheapAsAMove as
MaybeAsCheapAsAMove. I agree with this understanding and attached are the
updated two patches to reflect two more changes,

1) Add isAsCheapAsAMove flag in .td file for those instructions where I
added isRematerializable.
2) Promte all MachineInstr::IsAsCheapAsAMove to
TargetInstrInfo::isAsCheapAsAMove.

Thanks,
-Jiangning



2014-07-11 17:12 GMT+08:00 Quentin Colombet <qcolombet at apple.com>:

> Hi Jiangning,
>
> On Jul 10, 2014, at 8:17 PM, Jiangning Liu <liujiangning1 at gmail.com>
> wrote:
>
> Hi Quentin,
>
>>
>> Going back to the concern that we are conflating isRematerializable and
>> isAsCheapAsAMove, I can see why we want to keep both. That said, I still
>> think that if we are overriding isAsCheapAsMove with this new hook for
>> isRematerializable instructions, those instructions should be marked as
>> isAsCheapAsMove too.
>>
>> This means two things:
>> 1. All the uses of MachineInstr::isAsCheapAsMove should be promoted to
>> use of TargetInstrInfo::isAsCheapAsMove.
>> 2. If an instruction is rematerializable but not as cheap as move, then
>> it shouldn’t appear in the isAsCheapAsMove target hook.
>>
>> If you have an instruction matching #2 and still want to make it appear
>> in TargetInstrInfo::isAsCheapAsMove, it means you are trying to abuse the
>> register coalescer to do rematerialization and that’s probably not the
>> right solution.
>>
>> What do you think?
>>
>
> For case #2, you are right, so if we want to make make it
> rematerializable, actually we could
> override isReallyTriviallyReMaterializable(MI, AA) instead.
>
>     /// isTriviallyReMaterializable - Return true if the instruction is
> trivially
>     /// rematerializable, meaning it has no side effects and requires no
> operands
>
>
>     /// that aren't always available.
>     bool isTriviallyReMaterializable(const MachineInstr *MI,
>                                      AliasAnalysis *AA = nullptr) const {
>       return MI->getOpcode() == TargetOpcode::IMPLICIT_DEF ||
>               (MI->getDesc().isRematerializable() &&
>               (isReallyTriviallyReMaterializable(MI, AA) ||
>                isReallyTriviallyReMaterializableGeneric(MI, AA)));
>     }
>
> For register coalesce pass, so far the algorithm could only rematerialize
> instructions that are as cheap as move, because of the code in register
> coalesce pass I pasted previously,
>
>   if (!DefMI->isAsCheapAsAMove())
>     return false;
>   if (!TII->isTriviallyReMaterializable(DefMI, AA))
>
>
>
>     return false;
>
> If we want to change it, I'd suggest doing it separately.
>
>
> No, the register coalescer does the right thing. It rematerializes to
> remove a copy if and only if the rematerialized instruction is as cheap as
> a move.
> The rematerialization flag alone is used by the spiller to avoid spill
> code. The assumption there is that spill code is more expensive than
> anything else.
>
>
> If we add flag isAsCheapAsMove in .td file, we needn't to override
> TargetInstrInfo::isAsCheapAsMove() at all for this patch.
>
>
> Why not?
> After all, some instructions are as cheap as move only with certain
> argument/micro-architecture and the target hook seems appropriate.
>
> Previously I proposed this solution at
> http://comments.gmane.org/gmane.comp.compilers.llvm.cvs/194325, and Tim
> gave me feedback of not adding micro-architecture dependent things like
> this in .td file, and I was convinced this is a good design level choice.
> After all, for other micro-architectures, the instructions I'm highlighting
> right now may not be as cheap as move.
>
>
> To me, promoting all uses of MachineInstr::isAsCheapAsMove to
> TargetIntrInfo::isAsCheapAsMove is the right approach. Assuming we document
> the fact that isAsCheapAsMove now means isAsCheapAsMove plus some
> flexibility is we want to teach that in the related target hook.
> Alternatively we could come up with a new flag, e.g., mayBeAsCheapAsMove,
> but I think it will confuse the developer instead of clarifying the
> situation.
>
> Anyway, I am open to any suggestion as long as is we use a target hook
> called XXX we have the related XXX flag on the instruction, like we do with
> isCommutable.
>
> Thanks,
> -Quentin
>
>
> Attached is the patch making the small change as you suggested.
>
> Thanks,
> -Jiangning
>
>
>>
>> Cheers,
>> -Quentin
>>
>>
> <0002-Implement-AArch64-TTI-interface-isAsCheapAsAMove.patch>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140716/c4f26cf5/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-TargetInstrInfo-interface-isAsCheapAsAMove.patch
Type: application/octet-stream
Size: 6410 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140716/c4f26cf5/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Implement-AArch64-TTI-interface-isAsCheapAsAMove.patch
Type: application/octet-stream
Size: 5986 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140716/c4f26cf5/attachment-0001.obj>