[PATCH] D71521: Support for library function aligned_alloc

[Uday Bondhugula via Phabricator via llvm-commits]

bondhugula marked 4 inline comments as done.
bondhugula added a comment.

Thanks for the comments. Responses inline.



================
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?
I didn't pay attention to this part myself beyond reusing from malloc handling. Looks like the values being compared are all unsigned or size_t.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4317
+            return true;
+          }
     } else if (IsCalloc) {
----------------
jdoerfert wrote:
> 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?
The 'alloca' instruction takes a constant alignment (both from the langref and the AllocaInst ctors). Also, a dynamic alignment would be tantamount to a dynamic sized allocation, which we aren't converting to stack allocations. (The padding needed for a dynamic alignment could effectively lead to an allocation of at least that size in the worst case.)


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4317
+            return true;
+          }
     } else if (IsCalloc) {
----------------
bondhugula wrote:
> jdoerfert wrote:
> > 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?
> The 'alloca' instruction takes a constant alignment (both from the langref and the AllocaInst ctors). Also, a dynamic alignment would be tantamount to a dynamic sized allocation, which we aren't converting to stack allocations. (The padding needed for a dynamic alignment could effectively lead to an allocation of at least that size in the worst case.)
> Shouldn't the `isa<ConstantInt>(I.getOperand(0)` be inside the `if (IsAlignedAllocLike)`?

It'd be the same that way, but you'll need an extra line and indent for the rest. I can drop the comment under.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4356
   }
 };
 
----------------
jdoerfert wrote:
> 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.
This patch is all about support for aligned_alloc in analyses/transforms on par with malloc/calloc, and so I felt it would be incomplete if we split up some of that. We are making a similar impact on InstCombine, GVN, NewGVN, and BasicAliasAnalysis, and so logically this should all be together?


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

https://reviews.llvm.org/D71521