[PATCH] D21704: [PM] Port float2int to the new pass manager

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

Thanks, David and Davide.


================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:73
@@ -88,1 +72,3 @@
+                      false)
 INITIALIZE_PASS_DEPENDENCY(GlobalsAAWrapperPass)
+INITIALIZE_PASS_END(Float2IntLegacyPass, "float2int", "Float to int", false,
----------------
davide wrote:
> 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.
Right, I noticed that too, but forgot about it. I'll remove it.

================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:537
@@ +536,3 @@
+PreservedAnalyses Float2IntPass::run(Function &F, FunctionAnalysisManager &) {
+  //FIXME: skipFunction is not currently supported in the new PM.
+  if (!Impl.runImpl(F))
----------------
davide wrote:
> davidxl wrote:
> > This is a good point. Is there a bug tracking this missing functionality?
> I'd remove the FIXME. No other pass mentions it, and I'm not sure if there's active work on this.
You're probably right. 
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.

================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:541
@@ +540,3 @@
+  else {
+    //FIXME: setPreservesCFG is not currently supported in the new PM.
+    PreservedAnalyses PA;
----------------
davide wrote:
> davidxl wrote:
> > same here.
> I don't think there's a bug tracker for this (probably there should be one tho?). 
> 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. 
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.


http://reviews.llvm.org/D21704