<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 24, 2016 at 3:30 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">mkuper added a comment.<br>
<br>
Thanks, David and Davide.<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/Float2Int.cpp:73<br>
@@ -88,1 +72,3 @@<br>
+                      false)<br>
 INITIALIZE_PASS_DEPENDENCY(GlobalsAAWrapperPass)<br>
+INITIALIZE_PASS_END(Float2IntLegacyPass, "float2int", "Float to int", false,<br>
----------------<br>
</span><span class="">davide wrote:<br>
> This is not your bug, but still worth mentioning. This pass preserves globalsAA, doesn't depend on it. In other words, this can go away.<br>
</span>Right, I noticed that too, but forgot about it. I'll remove it.<br>
<span class=""><br>
================<br>
Comment at: lib/Transforms/Scalar/Float2Int.cpp:537<br>
@@ +536,3 @@<br>
+PreservedAnalyses Float2IntPass::run(Function &F, FunctionAnalysisManager &) {<br>
+  //FIXME: skipFunction is not currently supported in the new PM.<br>
+  if (!Impl.runImpl(F))<br>
----------------<br>
</span><span class="">davide wrote:<br>
> davidxl wrote:<br>
> > This is a good point. Is there a bug tracking this missing functionality?<br>
> I'd remove the FIXME. No other pass mentions it, and I'm not sure if there's active work on this.<br>
</span>You're probably right.<br>
I remember some talk about this possibly being handled by the PM, so that run() methods won't even be called when the function should be skipped. I'll remove the FIXME, thanks.<br></blockquote><div><br></div><div>I have filed: <a href="https://llvm.org/bugs/show_bug.cgi?id=28316">https://llvm.org/bugs/show_bug.cgi?id=28316</a></div><div>And added it as a dependency for <a href="https://llvm.org/bugs/show_bug.cgi?id=28315">https://llvm.org/bugs/show_bug.cgi?id=28315</a> (I just created that and will be filling it in shortly)</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<span class=""><br>
================<br>
Comment at: lib/Transforms/Scalar/Float2Int.cpp:541<br>
@@ +540,3 @@<br>
+  else {<br>
+    //FIXME: setPreservesCFG is not currently supported in the new PM.<br>
+    PreservedAnalyses PA;<br>
----------------<br>
</span><span class="">davide wrote:<br>
> davidxl wrote:<br>
> > same here.<br>
> I don't think there's a bug tracker for this (probably there should be one tho?).<br>
> As a general comment, this needs an understanding which passes actually now 'preserve the CFG' (whatever that means, we need to agree on a sane/clear semantic) so I'm not entirely sure the changes here will be NFC.<br>
</span>I don't know about a bug tracker, but there are FIXMEs for this in many ported passes. I think it's good to have these fixmes regardless of what exactly happens in the future, simply to serve as (another) marker for passes where it makes *some* sense.<br>
<br>
<br>
<a href="http://reviews.llvm.org/D21704" rel="noreferrer" target="_blank">http://reviews.llvm.org/D21704</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>