<div dir="ltr"><div><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Feb 25, 2015 at 1:04 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><span class=""><br><div class="gmail_quote">On Mon, Feb 23, 2015 at 4:01 PM, Kostya Serebryany <span dir="ltr"><<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Indeed, Chandler, I actually don't see why this could lead to false positives? What do we miss?</blockquote></div><br></span>I didn't realize that Anna was using the literal 'isAllocaPromotable' function and nothing else. That much is safe.</div><div class="gmail_extra"><br></div><div class="gmail_extra">That function's name is a bit of a lie, we actually promote wildly more than what it accepts. That's what got me confused. Looking at the patch, the comments in the code also seemed confused -- it talks about these being "unlikely" when it should say "impossible". =]</div><div class="gmail_extra"><br></div></div></blockquote><div><br></div><div>Good point. I'll update the comment. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"></div><div class="gmail_extra"><br></div><div class="gmail_extra">Meta point: I find the 2 separate review threads very frustrating to follow at this point though, so if you want further discussion it would be much more useful to focus it in a single place.</div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">While this patch is a fine initial step, I continue to think that refactoring this stuff the way I proposed, and handling bounds checks much as discussed by Nuno and others, is the right long term strategy. The benefit provided here will be gotten for free as part of that along with *many* more benefits. I understand that this is much more work, but I don't want people to stop thinking about this because the easy cases at -O0 have been handled.</div></div></blockquote><div><br></div><div>I agree with this long term strategy. I propose to continue the discussion on the other thread. Dmitry is making steps in the right direction.</div><div><br></div><div>I'd like to commit this even though it might be obsolete after the more aggressive analysis make it in since I'd like to have something that has immediate benefits at -O0 and is on by default. We can re-evaluate having this after the other patch is more mature. </div></div><br></div></div>