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.
Details
- Reviewers
nemanjai stefanp lei - Group Reviewers
Restricted Project - Commits
- rG70391b3468b8: [PowerPC] FP compare and test XL compat builtins.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Adding SemaChecking for the first argument of __builitn_ppc_test_data_class so the llvm_unreachable in CGBuiltin is actually unreachable.
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
3492 | 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 |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
3542 | I don't think this need to be function in the Sema class. It can just be a static function. | |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
10381 | 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. |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
3542 | 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. |
Separating the switch case in ISelLowering into two switch cases. One for compare_exp_ and another for test_data_class.
clang/include/clang/Sema/Sema.h | ||
---|---|---|
12680 ↗ | (On Diff #374312) | need to remove. |
clang/lib/CodeGen/CGBuiltin.cpp | ||
16081–16082 | ArgValue is only used one so not needed. llvm::Type *ArgType = EmitScalarExpr(E->getArg(0))->getType(); | |
16083 | variables should be discriptive of what they represent. This is no diff then a single char variable 🙂 unsigned IntrinsicID; | |
16084 | braces here are redundant. | |
16092 | Try to refrain from def one-time use variables. return Builder.CreateCall(CGM.getIntrinsic(Int), Ops, "test_data_class"); | |
clang/lib/Sema/SemaChecking.cpp | ||
3494 | I'm not sure this is needed... can't we just return true here since this is a S error? | |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
10382–10383 | I dont' think this is needed since you will only be here if he IntrinsicID matches the lines listed prior to this block. | |
10397–10398 | one time used variables can be removed. | |
10408–10417 | 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) CmprOpc = PPC::XSTSTDCSP; | |
10419 | one-time use variable. Can be merged into the call below. |
LGTM
Pleases address nit on commit.
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
3497–3498 | nit: I think these 2 stmts can just be merged into return Diag(....); and brace removed. |
ArgValue is only used one so not needed.