<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, May 24, 2016 at 3:14 PM, Michael Kuperstein <span dir="ltr"><<a href="mailto:mkuper@google.com" target="_blank">mkuper@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">mkuper added inline comments.<br>
<br>
================<br>
<span class="">Comment at: lib/Analysis/BasicAliasAnalysis.cpp:998<br>
@@ -997,3 @@<br>
-        // assert.<br>
-        if (GEP1BasePtr != UnderlyingV1 || GEP2BasePtr != UnderlyingV2) {<br>
-          return MayAlias;<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> mkuper wrote:<br>
> > davidxl wrote:<br>
> > > Extract this unrelated cleanup into  separate patch.<br>
> > It's not exactly unrelated.<br>
> ><br>
> > The old code would call DecomposeGEPExpression on-demand only in code paths that were using the decomposition. Which was, I think, the majority - but not 100%. I needed to hoist the calls up - but I don't think that should go in as a stand-alone patch, since if it goes in without the rest of my changes, it incurs compile-time cost without much benefit.<br>
> ><br>
> > If you think the clean-up is worth that, I can split it into a separate patch, but I'd rather not.<br>
> I mean the part that turns return MayAlias into assert -- not the hoisting part of the change. I think you just need to make that change  (lgtm to that part which can be committed first) and rebase this patch. Or perhaps just leaves the FIXME in this patch (in the hoisted code and clean it up as a follow up.<br>
</span>Oh, yes, that makes perfect sense, but is slightly more annoying than it looks.<br>
<br>
In fact, I already committed that as a separate patch a few days ago (r270268) - but reverted it because of unused variable warnings in non-asserts builds. I thought about recommitting it with dummy accesses to silence the warnings, but that seemed silly. (And I can't put the calls under NDEBUG, because only the result of the call is unused, but the output parameters aren't).<br>
<br>
Leaving the FIXME in this patch could potentially change behavior, since it would return MayAlais on different code paths, even those that originally didn't care about the bound at all. On the other hand, it should never actually fire, so that should be fine.<br>
<br>
I'm not too excited about either option, but if you prefer one of them to the whole thing going in in one piece, let me know which one.<br>
<br></blockquote><div><br></div><div>Ah -- I see -- you have already gone though exercise  -- so it is fine to leave it here.</div><div><br></div><div>David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<a href="http://reviews.llvm.org/D20495" rel="noreferrer" target="_blank">http://reviews.llvm.org/D20495</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>