[PATCH] D80422: Enable `align <n>` to be used in intrinsic definitions.
Johannes Doerfert via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 26 15:17:55 PDT 2020
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
More information about the llvm-commits
mailing list