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

[Jiangning Liu]

Ping^2...


2014-07-21 11:12 GMT+08:00 Jiangning Liu <liujiangning1 at gmail.com>:

> Ping...
>
>
> 2014-07-16 11:27 GMT+08:00 Jiangning Liu <liujiangning1 at gmail.com>:
>
> 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/20140723/5948dded/attachment.html>