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

[Jiangning Liu]

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/20140721/b762f176/attachment.html>