[PATCH] D71521: Support for library function aligned_alloc

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a subscriber: sstefan1.
jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:9
 //
-// This file implements an inter procedural pass that deduces and/or propagating
+// This file implements an interprocedural pass that deduces and/or propagates
 // attributes. This is done in an abstract interpretation style fixpoint
----------------
You can commit the wording/spelling fixes in the Attributor without additional review.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4144
 
+      MaybeAlign alignment;
       Constant *Size;
----------------
Upper case letter please: `Alignment`.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4305
       if (auto *Size = dyn_cast<ConstantInt>(I.getOperand(0)))
         if (Size->getValue().sle(MaxHeapToStackSize))
           if (UsesCheck(I) || FreeCheck(I)) {
----------------
lebedev.ri wrote:
> Hm, why are these comparisons signed?
Good question. I doubt we have a test for this. @sstefan1 Can you comment on this, and/or write a test and make it unsigned?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4317
+            return true;
+          }
     } else if (IsCalloc) {
----------------
Shouldn't the `isa<ConstantInt>(I.getOperand(0)` be inside the `if (IsAlignedAllocLike)`?

Do we really want to skip on heap 2 stack if the alignment is dynamic?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4356
   }
 };
 
----------------
Can we split the aligned alloc stuff from the heap 2 stack stuff? I think it is nicer and easier if we don't merge them.


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

https://reviews.llvm.org/D71521