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

[Michael Kuperstein via llvm-commits]

mkuper added inline comments.

================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:526
@@ +525,3 @@
+
+bool Float2IntLegacyPass::runOnFunction(Function &F) {
+  if (skipFunction(F))
----------------
davide wrote:
> This function can be inlined (line 60), no?
> `   bool runOnFunction(Function &F) override;`
> 
Sure, I don't feel strongly about it either way. Will do.

================
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:
> 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?


http://reviews.llvm.org/D21704