[PATCH] D61810: [BPF] add new intrinsics preserve_{array,union,struct}_access_index

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: docs/LangRef.rst:16961
+      @llvm.preserve.array.access.index.p0s_union.anons.p0a10s_union.anons(<type> base,
+                                                                           i8 *inst_name,
+                                                                           i32 subscript)
----------------
inst_name seems unnecessary.

Don't you need some argument to indicate the type of the array elements?


================
Comment at: docs/LangRef.rst:16992
+      @llvm.preserve.union.access.index.p0s_union.anons.p0s_union.anons(<type> base,
+                                                                        i8 *type_name,
+                                                                        i32 di_index)
----------------
How do you plan to use this?


================
Comment at: docs/LangRef.rst:17025
+                                                                 i32 gep_index,
+                                                                 i32 di_index)
+
----------------
I'm not sure about depending on digging the necessary information out of debug info, as opposed to generating some dedicated data structure.  It feels more complicated, since it's not clear what parts of the debug info, exactly, you're depending on.

Looking up the struct type by name seems error-prone; if you're depending on the debug info anyway, could you pass the debug info for the type directly as a metadata argument?


================
Comment at: include/llvm/IR/IRBuilder.h:2270
 
+  Value *CreatePreserveArrayAccessIndex(Value *Base, const StringRef &Name,
+                                        unsigned Index) {
----------------
Don't pass StringRefs by const reference (see https://llvm.org/docs/ProgrammersManual.html#the-stringref-class ).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61810