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

[Quentin Colombet]

Hi Jiangning,

The patch looks good with one nitpick however, I think we need a bit more discussion before we commit both patches.

First the nitpick:
+  case AArch64::ORNWrr:
+  case AArch64::ORNXrr:
+  case AArch64::ORRWrr:
+  case AArch64::ORRXrr:
+    return true;
+  }
// <— use llvm_unreachable here, just to make sure we do not miss that when updating the code.
+}

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?

Cheers,
-Quentin

> On Jul 9, 2014, at 6:36 PM, Jiangning Liu <liujiangning1 at gmail.com> wrote:
> 
> Hi Quentin,
> 
> Updated with the latest comments.
> 
> BTW, for instructions updating NCZV flags, actually I think it still make
> sense to mark them as rematerializable. Essentially the algorithm of
> rematerialization should take care of the final decision. For example, an
> adds instruction can still be rematerialized if only the flag is not really
> used by any instruction at all on control flow, or the flag may be
> overridden by another adds instruction. But I think it's OK to do it
> conservatively at this moment, so I agree to remove all instructions
> updating flags.
> 
> Thanks,
> -Jiangning
> 
> 2014-07-09 22:28 GMT+08:00 Quentin Colombet <qcolombet at apple.com>:
> 
>> Hi Jiangning,
>> 
>> A few comments on the second patch (the first one looks good).
>> 
>> +// FIXME: this implementation should be micro-architecture dependent, so a
>> +// micro-architecture target hook should be introduced here in future.
>> +bool AArch64InstrInfo::isAsCheapAsAMove(const MachineInstr *MI) const {
>> +  if (Subtarget.isCortexA57() || Subtarget.isCortexA53()) {
>> +    switch (MI->getOpcode()) {
>> +    default:
>> +      return false;
>> […]
>> +  }
>> +
>> +  return MI->isAsCheapAsAMove();
>> +}
>> +
>> 
>> To match LLVM guidelines, use an early exit when the subtarget does not
>> match:
>> if (!Subtarget.isCortexA57() && !Subtarget.isCortexA53())
>>  return MI->isAsCheapAsAMove();
>> switch (MI->getOpcode()) {
>> […]
>> 
>> +    case AArch64::ANDSWri:
>> +    case AArch64::ANDSXri:
>> 
>> I believe this is a remaining of the initial patch. The S variant are not
>> rematerializable.
>> 
>> Thanks,
>> -Quentin
>> 
>> http://reviews.llvm.org/D4361
>> 
>> 
>> 
> 
> - {F94052, layout=link}
> 
> http://reviews.llvm.org/D4361

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140710/720d5823/attachment.html>