[PATCH] D116851: [MemoryBuiltins] Add field for alignment argument

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/include/llvm/Analysis/MemoryBuiltins.h:75
     const Value *V, function_ref<const TargetLibraryInfo &(Function &)> GetTLI);
+/// Gets the alignment argument for an aligned_alloc-like function
+int getAllocAlignmentArg(const Value *V, const TargetLibraryInfo *TLI);
----------------
Move this down to the new property section please.


================
Comment at: llvm/include/llvm/Analysis/MemoryBuiltins.h:76
+/// Gets the alignment argument for an aligned_alloc-like function
+int getAllocAlignmentArg(const Value *V, const TargetLibraryInfo *TLI);
 
----------------
I'd suggest changing the return value from the operand index to the Value * of the operand or nullptr.  This seems like it would cleanup the current client code a bit, and would also allow us to return constant exprs for any allocation function whose alignment is a (known) function of the constant allocated size.  I'm not suggesting you do the last bit, just that you leave the potential open. 


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:5957
         Alignment = max(Alignment, RetAlign);
-      if (AI.Kind == AllocationInfo::AllocationKind::ALIGNED_ALLOC) {
+      if (AI.AlignArgPos != -1) {
         Optional<APInt> AlignmentAPI =
----------------
I think this block would be cleaner as:
if (isAllocationfn(*AI.CB)) {
  Value *Align = getAlignment(*AI.CB)
  ..
}

This lets you drop all the passing around indices.  


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:6268
     if (MaxHeapToStackSize == -1) {
-      if (AI.Kind == AllocationInfo::AllocationKind::ALIGNED_ALLOC)
-        if (!getAPInt(A, *this, *AI.CB->getArgOperand(0)).hasValue()) {
+      if (AI.AlignArgPos != -1)
+        if (!getAPInt(A, *this, *AI.CB->getArgOperand(AI.AlignArgPos))
----------------
Same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116851