This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Add max/min intrinsics to Clang and PPC backend
ClosedPublic

Authored by tingwang on Mar 25 2022, 6:22 AM.

Details

Summary

Add support for builtin_[max|min] which has below prototype:
A
builtin_max (A1, A2, A3, ...)
All arguments must have the same type; they must all be float, double, or long double.

Internally use SelectCC to get the result.

Diff Detail

Event Timeline

tingwang created this revision.Mar 25 2022, 6:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2022, 6:22 AM
tingwang requested review of this revision.Mar 25 2022, 6:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2022, 6:22 AM

Since this is compatibility support, I'm trying to match the result from XLC in scenarios where there is all kinds of QNaN, SNaN, +/-Infinity, +/-ZERO. Currently maxfl and maxfs still give different result compared with XLC in above scenario. This is one thing I'm still looking into.

tingwang updated this revision to Diff 418485.Mar 27 2022, 7:54 PM

Option -mlong-double-128 is not supported on AIX currently, and clang fails due to type mismatch in the fe case. Add check logic to print diag message in this case.

qiucf added a subscriber: qiucf.Mar 28 2022, 12:08 AM
qiucf added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
9899 ↗(On Diff #418485)

Can we re-use err_typecheck_convert_incompatible?

clang/lib/CodeGen/CGBuiltin.cpp
16305–16325
clang/lib/Sema/SemaChecking.cpp
3913

I think we don't need this fixme.

clang/test/CodeGen/PowerPC/builtins-ppc.c
65

Don't break CHECK lines.

clang/test/Sema/builtins-ppc.c
13–17
63–83

I don't know if it's convention in tests, but looks simpler.

llvm/include/llvm/IR/IntrinsicsPowerPC.td
192

Will we support llvm_f128_ty?

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
10572–10576

We can make it even simpler.

10583

We don't need to mention old behavior.

10584–10585
10587–10591

I don't think we need to manually call LowerSELECT_CC here. SelectionDAG knows ppc_fp128 should not be custom lowered.

This also makes the case pass. Thus D122462 is not needed.

11251

Why only two fe?

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-maxmin.ll
3

Can we add pwr8 run line?

tingwang updated this revision to Diff 418509.Mar 28 2022, 12:56 AM

Update test case to show that we need D122462 to fix the crash.

tingwang updated this revision to Diff 418544.Mar 28 2022, 4:38 AM
tingwang marked 11 inline comments as done.

Update based on Chaofan's suggestions.

llvm/include/llvm/IR/IntrinsicsPowerPC.td
192

I'm afraid not at this moment. Document mentions only three types: float, double, or long double.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
10587–10591

Thank you for pointing out this. Verified LowerSELECT_CC is not required here. This was added due to misconception.

11251

This is only for ppcf128 during type legalization: DAGTypeLegalizer::ExpandFloatResult -> CustomLowerNode -> PPCTargetLowering::ReplaceNodeResults. The other cases seem not hitting here. I will double check the code path to verify.

tingwang edited the summary of this revision. (Show Details)Mar 28 2022, 4:41 AM
jsji added a reviewer: qiucf.Mar 28 2022, 6:58 AM
qiucf added inline comments.Apr 1 2022, 1:43 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
9897 ↗(On Diff #418544)

Use err_target_unsupported_type?

llvm/include/llvm/IR/IntrinsicsPowerPC.td
192

Can we at least leave a TODO comment here for llvm_f128_ty support?

tingwang updated this revision to Diff 419704.Apr 1 2022, 3:55 AM

Update based on comments:
(1) Reuse diag error message.
(2) Update clang test case for those diag messages.
(3) Add TODO comment.

tingwang marked an inline comment as done.Apr 1 2022, 3:56 AM
tingwang added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
9897 ↗(On Diff #418544)

Thanks again for pointing it out!

qiucf accepted this revision as: qiucf.Apr 5 2022, 9:03 AM

Looks good to me in my side. Thanks for implementing it.

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