Page MenuHomePhabricator

[Intrinsic] Rename flt.rounds intrinsic to get.rounding
ClosedPublic

Authored by qiucf on Dec 6 2022, 10:25 PM.

Details

Summary

This is to address the inconsistency between FLT_ROUNDS_ and SET_ROUNDING SDAG node. Rename FLT_ROUNDS_ to GET_ROUNDING and add llvm.get.rounding intrinsic to replace flt.rounds.

Diff Detail

Event Timeline

qiucf created this revision.Dec 6 2022, 10:25 PM
qiucf requested review of this revision.Dec 6 2022, 10:25 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptDec 6 2022, 10:25 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thank you for working on this!

Is there any reason why we should keep the old intrinsic?

Thank you for working on this!

Is there any reason why we should keep the old intrinsic?

I'm not clear on the general policy, but for x86 we tend to provide bc of old intrinsics/sigtaures via AutoUpgrade.cpp, supposedly only for a few releases but tbh we remove them very rarely....

qiucf added a comment.Dec 7 2022, 11:43 PM

Thank you for working on this!

Is there any reason why we should keep the old intrinsic?

In case any user outside of clang references it (although I believe no), we can deprecate it and remove after a few releases.

LGTM.

Thank you for working on this!

Is there any reason why we should keep the old intrinsic?

In case any user outside of clang references it (although I believe no), we can deprecate it and remove after a few releases.

Yes, this could be a good solution.

sepavloff accepted this revision.Dec 8 2022, 12:21 AM
nikic requested changes to this revision.Dec 8 2022, 12:25 AM
nikic added a subscriber: nikic.

Thank you for working on this!

Is there any reason why we should keep the old intrinsic?

In case any user outside of clang references it (although I believe no), we can deprecate it and remove after a few releases.

This is not how intrinsic changes work. You need to add AutoUpgrade support.

This revision now requires changes to proceed.Dec 8 2022, 12:25 AM
qiucf updated this revision to Diff 482381.Dec 13 2022, 12:43 AM
qiucf retitled this revision from [Intrinsic] Add get.rounding as alias to flt.rounds and rename related DAG nodes to [Intrinsic] Rename flt.rounds intrinsic to get.rounding.
qiucf edited the summary of this revision. (Show Details)

Use AutoUpgrade to rename flt.rounds to get.rounding.

nikic added inline comments.Dec 13 2022, 12:48 AM
libcxxabi/test/test_demangle.pass.cpp
12917 ↗(On Diff #482381)

Demangling tests should not be updated.

llvm/docs/LangRef.rst
24567

Can drop this note now.

llvm/lib/IR/AutoUpgrade.cpp
916

This needs a test in llvm/test/Bitcode. Create a bitcode file using flt.rounds prior to this change, and then test llvm-dis on it.

nikic requested changes to this revision.Dec 15 2022, 2:53 AM

(per previous comment, mostly looks good though)

This revision now requires changes to proceed.Dec 15 2022, 2:53 AM
qiucf updated this revision to Diff 483434.Dec 15 2022, 10:48 PM
qiucf marked 3 inline comments as done.
qiucf removed a reviewer: Restricted Project.
qiucf removed a project: Restricted Project.
qiucf removed a subscriber: libcxx-commits.
nikic accepted this revision.Dec 16 2022, 12:26 AM

LGTM

This revision is now accepted and ready to land.Dec 16 2022, 12:26 AM
This revision was landed with ongoing or failed builds.Dec 18 2022, 11:23 PM
This revision was automatically updated to reflect the committed changes.