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

[Eric Christopher]

On Thu, Jul 10, 2014 at 6:51 AM, Quentin Colombet <qcolombet at apple.com> wrote:
> 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?
>

Thanks Quentin, this is exactly what I was worried about :)

I'll go back to lurking on the thread now.

-eric

> 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
>
>