This is an archive of the discontinued LLVM Phabricator instance.

[PM] Port float2int to the new pass manager
ClosedPublic

Authored by mkuper on Jun 24 2016, 3:01 PM.

Diff Detail

Event Timeline

mkuper updated this revision to Diff 61847.Jun 24 2016, 3:01 PM
mkuper retitled this revision from to [PM] Port float2int to the new pass manager.
mkuper updated this object.
mkuper added a subscriber: llvm-commits.
davidxl accepted this revision.Jun 24 2016, 3:15 PM
davidxl edited edge metadata.

lgtm

lib/Transforms/Scalar/Float2Int.cpp
537

This is a good point. Is there a bug tracking this missing functionality?

541

same here.

This revision is now accepted and ready to land.Jun 24 2016, 3:15 PM
davide added inline comments.Jun 24 2016, 3:22 PM
lib/Transforms/Scalar/Float2Int.cpp
73

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.

537

I'd remove the FIXME. No other pass mentions it, and I'm not sure if there's active work on this.

541

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.

Thanks, David and Davide.

lib/Transforms/Scalar/Float2Int.cpp
73

Right, I noticed that too, but forgot about it. I'll remove it.

537

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.

541

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.

davide edited edge metadata.Jun 24 2016, 3:35 PM

This one looks good modulo two small comments.

lib/Transforms/Scalar/Float2Int.cpp
526

This function can be inlined (line 60), no?
bool runOnFunction(Function &F) override;

541

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)

davide accepted this revision.Jun 24 2016, 3:35 PM
davide edited edge metadata.
mkuper added inline comments.Jun 24 2016, 3:48 PM
lib/Transforms/Scalar/Float2Int.cpp
526

Sure, I don't feel strongly about it either way. Will do.

541

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?

LGTM

lib/Transforms/Scalar/Float2Int.cpp
541

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?

mkuper added inline comments.Jun 24 2016, 4:26 PM
lib/Transforms/Scalar/Float2Int.cpp
541

SGTM.

This revision was automatically updated to reflect the committed changes.