Page MenuHomePhabricator

[PowerPC] FP compare and test XL compat builtins.

Authored by quinnp on Sep 8 2021, 7:42 AM.



This patch is in a series of patches to provide builtins for
compatability with the XL compiler. This patch adds builtins for compare
exponent and test data class operations on floating point values.

Diff Detail

Event Timeline

quinnp created this revision.Sep 8 2021, 7:42 AM
quinnp requested review of this revision.Sep 8 2021, 7:42 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 8 2021, 7:42 AM
quinnp added reviewers: Restricted Project, nemanjai, stefanp.Sep 8 2021, 7:52 AM
quinnp updated this revision to Diff 372243.EditedSep 13 2021, 6:56 AM

Adding SemaChecking for the first argument of __builitn_ppc_test_data_class so the llvm_unreachable in CGBuiltin is actually unreachable.

Conanap added inline comments.

If you're making a function anyways, it may be a good idea to just have a function with all the sema checking in it

lei added a subscriber: lei.Sep 20 2021, 2:21 PM
lei added inline comments.

I don't think this need to be function in the Sema class. It can just be a static function.


I think it'll be more clear if you move ppc_test_data-* handling out of this case stmt and move them into their own since they don't use the same CmprOpc or Op[1|2] as the other instrinsics.

kamaub added a subscriber: kamaub.Sep 21 2021, 10:47 AM
kamaub added inline comments.

This is a great suggestion, I'm wondering if the function is even needed at all though since the code block is quite small and only called once, having it inside the case statement seems more readable to me.

quinnp updated this revision to Diff 374064.Sep 21 2021, 4:20 PM

Separating the switch case in ISelLowering into two switch cases. One for compare_exp_ and another for test_data_class.

quinnp marked an inline comment as done.Sep 21 2021, 4:23 PM
quinnp updated this revision to Diff 374312.Sep 22 2021, 11:51 AM

Moving the semachecking of test_data_class from the function to the switch case.

quinnp marked 3 inline comments as done.Sep 22 2021, 11:52 AM
lei requested changes to this revision.Sep 23 2021, 7:09 AM
lei added inline comments.
12680 ↗(On Diff #374312)

need to remove.


ArgValue is only used one so not needed.

llvm::Type *ArgType = EmitScalarExpr(E->getArg(0))->getType();

variables should be discriptive of what they represent. This is no diff then a single char variable 🙂

unsigned IntrinsicID;

braces here are redundant.


Try to refrain from def one-time use variables.

return Builder.CreateCall(CGM.getIntrinsic(Int), Ops, "test_data_class");

I'm not sure this is needed... can't we just return true here since this is a S error?


I dont' think this is needed since you will only be here if he IntrinsicID matches the lines listed prior to this block.


one time used variables can be removed.


this can just be an if/else since you won't be in this block unless the IntrisicID are ppc_test_data_class_[d|f]

unsigned CmprOpc = PPC::XSTSTDCDP;
if (IntrinsicID == Intrinsic::ppc_test_data_class_f)

one-time use variable. Can be merged into the call below.

This revision now requires changes to proceed.Sep 23 2021, 7:09 AM
quinnp updated this revision to Diff 374605.Sep 23 2021, 10:18 AM

Addressing some review comments.

quinnp marked 9 inline comments as done.Sep 23 2021, 10:22 AM
quinnp updated this revision to Diff 374610.Sep 23 2021, 10:26 AM

Addressing review comment about ISelLowering of test_data_class.

quinnp marked an inline comment as done.Sep 23 2021, 10:26 AM
lei accepted this revision as: lei.Sep 24 2021, 8:51 AM

Pleases address nit on commit.


nit: I think these 2 stmts can just be merged into return Diag(....); and brace removed.

lei accepted this revision.Sep 24 2021, 8:52 AM
This revision is now accepted and ready to land.Sep 24 2021, 8:52 AM
quinnp updated this revision to Diff 374888.Sep 24 2021, 9:56 AM

Addressing nit in SemaChecking.

quinnp marked an inline comment as done.Sep 24 2021, 9:57 AM
quinnp updated this revision to Diff 374891.Sep 24 2021, 10:01 AM

Rebase with main.

This revision was landed with ongoing or failed builds.Sep 28 2021, 9:02 AM
This revision was automatically updated to reflect the committed changes.