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

[Quentin Colombet]

Hi Jiangning,

Thanks for working on this. That looks promising.

This is kind of strange that the patch introduces a target hook for isAsCheapAsMove, whereas it uses the flag isRematerializable to mark the interesting instructions.
Indeed, it seems kind of arbitrary which instructions are marked rematerializable and which instructions are not. If we want to pursue this way, I believe that we have to clarify the semantic of this flag.

The isRematerializable flag is also in a weird situation. Indeed, according to the documentation, it is deprecated:
  /// isRematerializable - Returns true if this instruction is a candidate for
  /// remat.  This flag is deprecated, please don't use it anymore.  If this
  /// flag is set, the isReallyTriviallyReMaterializable() method is called to
  /// verify the instruction is really rematable.
But it is used in TargetInstrInfo::isTriviallyRematerializable.

Since, you are touching this part, I believe that we should either fix the implementation of isTrivallyRematerializable and really deprecate this flag, or update the comment to reflect the actual semantic (which needs to be defined) of this flag.

Thanks,
-Quentin

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:564
@@ +563,3 @@
+    case AArch64::SUBXri:
+    case AArch64::ADDSWri:
+    case AArch64::ADDSXri:
----------------
Trying to rematerialize the S variant seems wrong to me. Those define the flags, so it is not safe to move them around.

Currently, it may not cause any problem as isTriviallyRematerializable checks that the instruction only defines one virtual register IIRC.
Anyway the logic looks wrong.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:568
@@ +567,3 @@
+    case AArch64::SUBSXri:
+      if (MI->getOperand(2).getImm() != 0)
+        return false;
----------------
I believe that the shifter immediate is at index 3, not 2:
def0 = ADDXri arg1, imm2, shifter3

=> Simply "return MI->getOperand(3).getImm() == 0;"?

BTW, is that test even correct from a performance perspective?
Indeed, in your example, you are rematerializing stack adjustments, but the value for stack adjustments is  known way after register coalescing (which does the re-materialization in your case). Therefore, something that seems cheap, may not be cheap after all.

http://reviews.llvm.org/D4361