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

[Maxim Ostapenko via llvm-commits]

m.ostepenko added inline comments.

================
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
----------------
hfinkel wrote:
> 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?
> offset is repeated twice here in a confusing way.
Ugh, sorry.

> We should define what we mean by "dynamic stack area" - is this one past the end of the last dynamic alloca?
Agreed.
Under "dynamic stack area" I mean the lowest address of local variable space after the last dynamic alloca. Frankly, I'm not sure "get dynamic stack area offset" is a good description for this intrinsic, any suggestions here are appreciated. 

================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:761
@@ +760,3 @@
+    /// nonzero constant. The only operand here is the chain.
+    GET_DYNAMIC_AREA_OFFSET,
+
----------------
hfinkel wrote:
> 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.
> 
Hm, I use //llvm_anyint_ty// in IR/Intrinsics.td, so I suspect this should be i32 for 32-bit targets and i64 for 64-bit ones. So, it seems that for most targets that would be pointer type, right?
>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.
If I understand you correctly, you mean what happens if someone add new custom lowering routine for some target, right? If someone declares this intrinsic with a wrong type, he would get //Expand// action in //Action = TLI.getOperationAction// during node legalization in //SelectionDAGLegalize::LegalizeOp//. Issuing an error in such a case would be nice, thought.

================
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);
----------------
hfinkel wrote:
> 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.
Ok.


Repository:
  rL LLVM

http://reviews.llvm.org/D14983