This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Use default attributes for intrinsics
ClosedPublic

Authored by nikic on Nov 8 2022, 3:47 AM.

Details

Reviewers
shchenz
Group Reviewers
Restricted Project
Commits
rGdeca64e2267e: [PowerPC] Use default attributes for intrinsics
Summary

This switches a large subset of PowerPC intrinsics to use default attributes (nosync, nofree, nocallback and willreturn). In particular the presence of willreturn is important to avoid optimization regression in the future.

This patch primarily covers readnone/readonly intrinsics.

Diff Detail

Event Timeline

nikic created this revision.Nov 8 2022, 3:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 3:47 AM
nikic requested review of this revision.Nov 8 2022, 3:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 3:47 AM
amyk added a subscriber: amyk.Nov 10 2022, 6:27 PM
amyk added inline comments.
llvm/include/llvm/IR/IntrinsicsPowerPC.td
1816

A question I had was, did we also mean to update these intrinsics here, as well?
Or are these excluded because these intrinsics have side effects?

nikic updated this revision to Diff 474709.Nov 11 2022, 3:09 AM

Use default attributes for mtfs intrinsics.

nikic added inline comments.Nov 11 2022, 3:11 AM
llvm/include/llvm/IR/IntrinsicsPowerPC.td
1816

Yeah, I skipped these originally due to the side effects. Looks like these are simple moves from/to fpscr, so default attribute should be fine for them as well.

Will there be some functionality issues without adding DefaultAttrsIntrinsic for intrinsics that are missing IntrNoMem attribute before? Or just performance issues?

I asked because if there are functionality issues, we may need to go through other intrinsics as well. I think there are some other intrinsics need change too, for example int_ppc_readflm and int_ppc_setflm.

nikic added a comment.Nov 15 2022, 7:52 AM

Will there be some functionality issues without adding DefaultAttrsIntrinsic for intrinsics that are missing IntrNoMem attribute before? Or just performance issues?

I asked because if there are functionality issues, we may need to go through other intrinsics as well. I think there are some other intrinsics need change too, for example int_ppc_readflm and int_ppc_setflm.

Missing default attributes only cause performance issues, it's not a problem for correctness. But of course, adding default attributes to other intrinsics is definitely useful as well, where possible.

shchenz accepted this revision.Nov 15 2022, 4:04 PM

LGTM. Thanks for doing this for PPC.

This revision is now accepted and ready to land.Nov 15 2022, 4:04 PM
This revision was landed with ongoing or failed builds.Dec 4 2022, 11:51 PM
This revision was automatically updated to reflect the committed changes.