[PATCH] D80422: Enable `align <n>` to be used in intrinsic definitions.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D80422#2055938 <https://reviews.llvm.org/D80422#2055938>, @arsenm wrote:

> Usually the attributes of a call are the union of (call site attributes, declaration attribute), but in this case it's a bit different since the call site will override it since it happens to be checked first. This is probably fine, but I'm also wondering if defining this as a minimum alignment might be better


Not sure I understand. If you query attributes you'll get the first that is found (I think), so if the call site provides one you get that, otherwise the function one, if present. I haven't seen anything here that would be "weird". Wrt. minimum alignment, I guess that is by definition the case. `align(X)` anywhere means minimum alignment and the Attributor would for example bump call site alignment to the function alignment if that is known to be higher.

I added some minor comments below. From my perspective this is fine, assuming I did understand @arsenm properly. If so, feel free to accept :)



================
Comment at: llvm/include/llvm/IR/Intrinsics.td:78
+  int ArgNo = argNo;
+  int Align = align;
+}
----------------
Nit: maybe `uint64_t` for the align to match other places, though it will work fine as long as `int` is 32 bit (or more).


================
Comment at: llvm/include/llvm/IR/IntrinsicsAMDGPU.td:173
   Intrinsic<[LLVMQualPointerType<llvm_i8_ty, 4>], [],
-  [IntrNoMem, IntrSpeculatable]>;
+  [Align<-1, 4>, IntrNoMem, IntrSpeculatable]>;
 
----------------
I think -1 is in principle the right thing here *but* we should try to make it a variable with a proper name, e.g. `ReturnIndex`.


================
Comment at: llvm/lib/IR/Attributes.cpp:1186
+  return get(C, Attrs);
+}
+
----------------
Nit: Message for the assert please.




================
Comment at: llvm/utils/TableGen/CodeGenIntrinsics.h:157
 
-  std::vector<std::pair<unsigned, ArgAttribute>> ArgumentAttributes;
+  std::vector<std::tuple<unsigned, ArgAttribute, uint64_t>> ArgumentAttributes;
 
----------------
Every time I see a use of tuple I ask myself the same question: Wouldn't a POD-like struct with some properly named members be better? I feel `std::get<1>(T)` is somewhat less helpful than `T->Index`. I don't need you to change it here but it's something to think about. In particular it might help to remove magic numbers below, e.g., 0 for the value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80422