[PATCH] D66012: [AArch64][Statepoints] Statepoint support for AArch64.

[Oliver Stannard (Linaro) via Phabricator via llvm-commits]

ostannard accepted this revision.
ostannard added a comment.

I don't know too much about statepoints or GC, but you have addressed all of @reames comments, and the AArch64 parts and code style look correct to me, so LGTM.



================
Comment at: llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp:456
 
   // Special handling of dbg_value, stackmap and patchpoint instructions.
   if (MI.isDebugValue() || MI.getOpcode() == TargetOpcode::STACKMAP ||
----------------
Comment should be updated.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:185-188
+  case Intrinsic::experimental_gc_statepoint:
+    if ((Idx < 5) || (Imm.getBitWidth() <= 64 && isInt<64>(Imm.getSExtValue())))
+      return TTI::TCC_Free;
+    break;
----------------
loicottet wrote:
> Not sure about this, I'd appreciate if someone could confirm this is the way to do it.
This looks right to me, none of the first 5 arguments need to be materialised as actual runtime values, so marking them as free is correct.


================
Comment at: llvm/test/CodeGen/AArch64/fast-isel-gc-intrinsics.ll:5
+target triple = "aarch64-unknown-linux-gnu"
+; Dont crash with gc intrinsics.
+
----------------
This test is only checking for crashes, is there anything in here which wouldn't be caught by the other tests?


================
Comment at: llvm/test/CodeGen/AArch64/statepoint-stack-usage.ll:8
+; for GC values spilled over two different call sites.  Since the order
+; of GC arguments differ, niave lowering code would insert loads and 
+; stores to rearrange items on the stack.  We need to make sure (for
----------------
naive


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66012/new/

https://reviews.llvm.org/D66012