This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Fix PPCISD::STBRX selection issue on A2
ClosedPublic

Authored by tingwang on May 8 2022, 9:08 PM.

Details

Summary

https://github.com/llvm/llvm-project/issues/55272 reported an error which appears to be one regression:

The troubled instruction was introduced for both P7 and A2 as below link shows:
https://github.com/llvm/llvm-project/commit/31d2956510b8484373fe244547b4f811430a28ff

However it seems below commit missed the A2 part and introduced the regression:
https://github.com/llvm/llvm-project/commit/fb4e44c4e7daaaa1d2776e76d43566b0db264f30

Referring to below page, it seems A2 should have FeatureISA2_06 in its feature list:
https://en.wikipedia.org/wiki/IBM_A2

Thus I'm proposing this fix. Please help comment, thanks!

Diff Detail

Event Timeline

tingwang created this revision.May 8 2022, 9:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2022, 9:08 PM
tingwang requested review of this revision.May 8 2022, 9:08 PM
lkail added a subscriber: lkail.May 8 2022, 9:54 PM

The troubled instruction was introduced for both P7 and A2 as below link shows:
https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20130325/169706.html

This link has been invalid.

tingwang edited the summary of this revision. (Show Details)May 8 2022, 11:31 PM

The troubled instruction was introduced for both P7 and A2 as below link shows:
https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20130325/169706.html

This link has been invalid.

Not sure what's going on there - the link works for me.

Why not also fix this in the front end so that we allow the builtin on the A2 CPU as well (since it's supported)?

tingwang updated this revision to Diff 428294.May 9 2022, 10:47 PM

Update according to Nemanja's comment: add A2 to frontend isa-v206-instructions feature list, together with test case update.

Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 10:47 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Why not also fix this in the front end so that we allow the builtin on the A2 CPU as well (since it's supported)?

Oh I missed that. Thank you for pointing out!

Just now updated the patch. However I didn't update the SemaFeatureCheck message to indicate the support on a2, since if people see this error message, it cannot be a2, and a2 does not easily fit into the message of err_ppc_builtin_only_on_arch. Hope this will not create problem.

nemanjai accepted this revision.May 10 2022, 7:44 AM

Why not also fix this in the front end so that we allow the builtin on the A2 CPU as well (since it's supported)?

Oh I missed that. Thank you for pointing out!

Just now updated the patch. However I didn't update the SemaFeatureCheck message to indicate the support on a2, since if people see this error message, it cannot be a2, and a2 does not easily fit into the message of err_ppc_builtin_only_on_arch. Hope this will not create problem.

I wouldn't worry about it. If we have to change this in the future because some implementation of the ISA that isn't pwr<N> needs to reference it, we can change it at that time.

LGTM.

This revision is now accepted and ready to land.May 10 2022, 7:44 AM
This revision was landed with ongoing or failed builds.May 10 2022, 5:48 PM
This revision was automatically updated to reflect the committed changes.