[PATCH] D65408: [Attributor] Heap-To-Stack Conversion

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D65408#1670708 <https://reviews.llvm.org/D65408#1670708>, @xbolva00 wrote:

> I think the current implementation needs some important fixes to avoid miscompilation/overflow, can you make it off by default, fix issues, and enable it again?


The Attributor is off by default (even though we should now pass LLVM-TS + SPEC2006 tests!!).

In D65408#1670703 <https://reviews.llvm.org/D65408#1670703>, @xbolva00 wrote:

> IsMallocLikeFn returns true for C++ operator new which does throw on an allocation failure.




In D65408#1670713 <https://reviews.llvm.org/D65408#1670713>, @xbolva00 wrote:

> [...] is probably converted to alloca. I have no strong opinion what should be done here, ideally warning! :)  Anyway, I would like to see some tests with C++'s new and various combinations (there are many variants of 'new', 'delete').


I fail to see the problem in converting a possible throwing `new` to a non-throwing `alloca`.
If you think there is one could you please try to explain why that should not be valid?

In D65408#1670703 <https://reviews.llvm.org/D65408#1670703>, @xbolva00 wrote:

> I think it is worth to add support for c++’s new - delete pairs in the future..


We can add this (the delete part) when we actually use free/delete to reason about h2s validity.

In D65408#1670713 <https://reviews.llvm.org/D65408#1670713>, @xbolva00 wrote:

> In D65408#1670705 <https://reviews.llvm.org/D65408#1670705>, @lebedev.ri wrote:
>
> > In D65408#1670702 <https://reviews.llvm.org/D65408#1670702>, @xbolva00 wrote:
> >
> > > Quite questionable is “stackification” of mallocs in loops, we should avoid it I think. @efriedma, what do you think?
> >
>
>
> Heap to stack idea started some time ago and @efriedma raised some concerns: https://reviews.llvm.org/D47345#1112573.


>From a pure standard point of view, I would argue the current implementation is not wrong in doing h2s for mallocs in loops.
I do however not intent to turn it on and let people deal with the aftermath. As mentioned by @sstefan1, this is not on by default.
My plan was to get the logic in, add the "must-be-freed"-based logic that @hfinkel used in the original version, and then look into heuristics.
The reason is that the "must-be-freed" actually allows different placement of the alloca and we want to see what is out there wrt. sizes etc.
Does that make sense and does my plan sound OK to you?



================
Comment at: llvm/trunk/lib/Transforms/IPO/Attributor.cpp:3363
+                   .sle(MaxHeapToStackSize))
+            if (!Overflow)
+              return true;
----------------
sstefan1 wrote:
> This was supposed to be `if(Overflow)`. I'll fix this.
Add a test for this please.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65408