[PATCH] D14983: Introduce new @llvm.getdynamicareaoffset.i{32, 64} intrinsic.

[hfinkel@anl.gov via llvm-commits]

hfinkel added a comment.

You need to add documentation for the intrinsic into the LangRef.


================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:758
@@ +757,3 @@
+    /// GET_DYNAMIC_AREA_OFFSET - get offset from native SP to the end of
+    /// dynamic stack area offset. For most targets that would be 0, but for
+    /// some others (e.g. PowerPC, PowerPC64) that would be compile time known
----------------
offset is repeated twice here in a confusing way. We should define what we mean by "dynamic stack area" - is this one past the end of the last dynamic alloca?

================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:759
@@ +758,3 @@
+    /// dynamic stack area offset. For most targets that would be 0, but for
+    /// some others (e.g. PowerPC, PowerPC64) that would be compile time known
+    /// nonzero constant. The only operand here is the chain.
----------------
compile time -> compile-time

================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:761
@@ +760,3 @@
+    /// nonzero constant. The only operand here is the chain.
+    GET_DYNAMIC_AREA_OFFSET,
+
----------------
What's the return type? Always the pointer type?

What happens if someone declares this with type i32 on a 64-bit system? We should not just randomly assert somewhere in this case; either handle it by doing an extend-or-truncate somewhere, or issue an error somewhere if the type does not match the pointer size type, and document the requirement in the LangRef.


================
Comment at: lib/CodeGen/IntrinsicLowering.cpp:428
@@ +427,3 @@
+  case Intrinsic::getdynamicareaoffset:
+    // Just lower it to a constant 0.
+    CI->replaceAllUsesWith(ConstantInt::get(Type::getInt64Ty(Context), 0));
----------------
This class is only used by the IR interpreter, as far as I can tell. While using 0 is probably not unreasonable, this comment does not explain why, and we should probably produce an error (as with returnaddress/frameaddress below).

================
Comment at: lib/CodeGen/TargetLoweringBase.cpp:886
@@ +885,3 @@
+  // For most targets @llvm.getdynamicareaoffest just returns 0.
+  for (MVT VT : {MVT::i32, MVT::i64, MVT::Other})
+    setOperationAction(ISD::GET_DYNAMIC_AREA_OFFSET, VT, Expand);
----------------
We do support targets with i16 pointers (etc.) -- using a fixed list here is not necessary. Just put this inside the `for (MVT VT : MVT::all_valuetypes())` loop that starts at the beginning of the function.


Repository:
  rL LLVM

http://reviews.llvm.org/D14983