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

[Quentin Colombet]

Hi Jiangning,

LGTM.

Thanks,
-Quentin

On Jul 22, 2014, at 11:10 PM, Jiangning Liu <liujiangning1 at gmail.com> wrote:

> 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/20140728/9bd252dd/attachment.html>