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

[Davide Italiano via llvm-commits]

davide added a comment.

LGTM


================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:541
@@ +540,3 @@
+  else {
+    //FIXME: setPreservesCFG is not currently supported in the new PM.
+    PreservedAnalyses PA;
----------------
mkuper wrote:
> davide wrote:
> > mkuper wrote:
> > > 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.
> > Yes a comment here makes sense, thanks! Can you please copy'n'paste from any of the passes I already ported (so that we have the same comment everywhere and it's easier to grep and/or locate when we want to replace)
> Unfortunately, it's already very non-uniform. We have at least:
> 
> $ find . -name *.cpp | xargs cat | grep // | grep -i preserve | grep CFG
>   // FIXME: Need a way to preserve CFG analyses here!
>   // FIXME: There is no setPreservesCFG in the new PM. When that becomes
>   // FIXME: This pass should preserve the CFG.
>   // FIXME: BDCE should also 'preserve the CFG'.
>     //FIXME: setPreservesCFG is not currently supported in the new PM.
>   // FIXME: ADCE should also 'preserve the CFG'.
>   // FIXME: once we have an equivalent of AU.setPreservesCFG() in the
>   // FIXME: This pass should also 'preserve the CFG'.
>     // FIXME: Reassociate should also 'preserve the CFG'.
> 
> I can try to find all of those - I'm sure there are some this grep missed - and replace with the same string (as a separate patch, of course), but there'd be no way to enforce it for future ports.
> Perhaps it would be better to have a dummy implementation that we can call?
I'm fine with the dummy implementation but I'd like to hear what David/Chandler/Sean have to say about it. Maybe you can just leave the comment as his and defer that to a different patch (maybe start a thread on llvm-dev)? How does that sound?


http://reviews.llvm.org/D21704