[CIR][CIRGen] Use Clang Codegen's skeleton in CIRGenFunction::buildBuiltinExpr #967

ghehg · 2024-10-13T20:29:53Z

This PR helps us to triage unimplemented builtins (that are target independent).
There are unhandled builtins in CIR Codegen [CIRGenFunction::buildBuiltinExpr](https://github.com/llvm/clangir/blob/4c446b3287895879da598e23164d338d04bced3e/clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp#L305).
And those builtins have implementation in OG.
Currently, those builtins just are treated as LibraryCall or some other ways which eventually get failure, and failure messages are confusing.
This PR address this problem by refactoring CIRGenFunction::buildBuiltinExpr to keep the same skeleton as OG
counterpart CodeGenFunction::EmitBuiltinExpr, and add builtin name to NYI message

smeenai

This is a great idea!

smeenai · 2024-10-14T04:03:32Z

clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp

+/// in OG code `CodeGenFunction::EmitBuiltinExpr`.
+/// NOTE: Every time we implement handling of a case in
+/// the function `CIRGenFunction::buildBuiltinExpr`, it should be remove here.
+static std::string getUnhandledBuiltinName(unsigned builtinID) {


This should return StringRef.

I'll use StringRef if we do return name.

smeenai · 2024-10-14T04:05:13Z

clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp

+  switch (builtinID) {
+  default:
+    return std::string();
+  case Builtin::BI__builtin_acos:


These builtins are defined in tablegen, and I usually search in my build directory to find the corresponding .inc file (tools/clang/include/clang/Basic/Builtins.inc in this case). Those files define all the builtins using macros, and you can provide an appropriate macro definition before including the file to autogenerate this switch instead of needing to manually write out every intrinsic.

Thanks for great advice. This situation is a bit different though because these builtin names are a selection of tools/clang/include/clang/Basic/Builtins.inc. They are actually a manual selection of builtins that has implementation in OG but not yet in CIRGen, And the idea was just to capture names and NYI early of those builtins (and them only) as those implemented in CIRGen should never hit the NYI I created here, and those not implemented in OG should go through and never hit NYI I created.
i.e. I used command like

"sed -n '2573,6333p' /data/users/guojinhe/clangir-llvm-ghehg/clang/lib/CodeGen/CGBuiltin.cpp | grep -o 'Builtin::BI__[^:]*' | sed 's/^/case /;s/$/:/' " to get them from oG.
and exclude what's from sed -n '305,1416p' /data/users/guojinhe/clangir-llvm-ghehg/clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp | grep "case Builtin:"
to pick them.

Yeah, let me know if we have better way to generate them

With the current implementation, I think it'd be okay to have this function handle all built-ins and not just unimplemented ones. Bruno's suggestion changes that, of course.

smeenai · 2024-10-14T04:05:31Z

clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp

+  std::string unhandledName = getUnhandledBuiltinName(BuiltinID);
+  if (!unhandledName.empty()) {
+    llvm::errs() << unhandledName << " ";
+    llvm_unreachable("NYI");


I think it'll be easier to parse the error if the unreachable message itself includes the builtin name.

bcardosolopes · 2024-10-14T18:01:47Z

clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp

+    return std::string();
+  case Builtin::BI__builtin_acos:
+    return "Builtin::BI__builtin_acos";
+  case Builtin::BI__builtin_acosf:


When you did similar for Aarch64 my feedback was to make those inline (without a function that returns the name), and you did implement like that. The idea is to get an accurate account of source line of failure, I'm confused on why you took a different approach here.

Example:

clangir/clang/lib/CIR/CodeGen/CIRGenBuiltinAArch64.cpp

Line 2410 in 81c043e

llvm_unreachable("NYI");

I see, sounds like better approach!
Let me see if I understand the tradeoff, because if we do have them inline instead of returning the name, not only we can have source line of failures (and name can be known by looking up source code), we would have a skeleton that we can fill when we have implementation ready. Plus, we don't need to keep and maintain this "return name" function as they may not be needed eventually. The only con with this alternative is every time we change the code, failing line number would change, but I think it might be Ok.

I'll change this PR to use this approach.

exactly, good point on the maintain bits too

bcardosolopes · 2024-10-17T21:26:58Z

This PR currently has no content, should probably be a draft to prevent generating noise if it's not ready for review

ghehg · 2024-10-18T01:28:46Z

Skeleton of OG introduced with informative NYIs.

bcardosolopes

This is awesome, many thanks!

…iltinExpr (llvm#967) This PR helps us to triage unimplemented builtins (that are target independent). There are unhandled builtins in CIR Codegen `[CIRGenFunction::buildBuiltinExpr](https://github.com/llvm/clangir/blob/4c446b3287895879da598e23164d338d04bced3e/clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp#L305)`. And those builtins have implementation in [OG](https://github.com/llvm/clangir/blob/4c446b3287895879da598e23164d338d04bced3e/clang/lib/CodeGen/CGBuiltin.cpp#L2573). Currently, those builtins just are treated as LibraryCall or some other ways which eventually get failure, and failure messages are confusing. This PR address this problem by refactoring `CIRGenFunction::buildBuiltinExpr` to keep the same skeleton as OG counterpart `CodeGenFunction::EmitBuiltinExpr`, and add builtin name to NYI message

…iltinExpr (#967) This PR helps us to triage unimplemented builtins (that are target independent). There are unhandled builtins in CIR Codegen `[CIRGenFunction::buildBuiltinExpr](https://github.com/llvm/clangir/blob/4c446b3287895879da598e23164d338d04bced3e/clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp#L305)`. And those builtins have implementation in [OG](https://github.com/llvm/clangir/blob/4c446b3287895879da598e23164d338d04bced3e/clang/lib/CodeGen/CGBuiltin.cpp#L2573). Currently, those builtins just are treated as LibraryCall or some other ways which eventually get failure, and failure messages are confusing. This PR address this problem by refactoring `CIRGenFunction::buildBuiltinExpr` to keep the same skeleton as OG counterpart `CodeGenFunction::EmitBuiltinExpr`, and add builtin name to NYI message

ghehg changed the title ~~[CIR][CIRGen]Let should-be-handled unhandled Builtins NYI with their name~~ [CIR][CIRGen] Let should-be-handled unhandled Builtins NYI with their name Oct 13, 2024

ghehg marked this pull request as ready for review October 14, 2024 01:32

ghehg requested review from lanza and bcardosolopes as code owners October 14, 2024 01:32

smeenai requested changes Oct 14, 2024

View reviewed changes

bcardosolopes requested changes Oct 14, 2024

View reviewed changes

ghehg force-pushed the second branch from c4b42a4 to c547fe4 Compare October 17, 2024 11:59

ghehg force-pushed the second branch from c547fe4 to 716b14a Compare October 18, 2024 01:22

ghehg changed the title ~~[CIR][CIRGen] Let should-be-handled unhandled Builtins NYI with their name~~ [CIR][CIRGen] Use Clang Codegen's skeleton in CIRGenFunction::buildBuiltinExpr Oct 18, 2024

ghehg added 4 commits October 18, 2024 05:05

NYI and print out unhandled builtin

8716844

Restore to previous version first

b289181

Keep skeleton of OG in CIRGenFunction::buildBuiltinExpr

07f2abf

all NYI with builtin name

43b5874

ghehg force-pushed the second branch from 716b14a to 43b5874 Compare October 18, 2024 12:05

bcardosolopes approved these changes Oct 18, 2024

View reviewed changes

bcardosolopes merged commit 25947d0 into llvm:main Oct 18, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CIR][CIRGen] Use Clang Codegen's skeleton in CIRGenFunction::buildBuiltinExpr #967

[CIR][CIRGen] Use Clang Codegen's skeleton in CIRGenFunction::buildBuiltinExpr #967

ghehg commented Oct 13, 2024 •

edited

Loading

smeenai left a comment

smeenai Oct 14, 2024

ghehg Oct 14, 2024

smeenai Oct 14, 2024

ghehg Oct 14, 2024

smeenai Oct 14, 2024

smeenai Oct 14, 2024

bcardosolopes Oct 14, 2024

bcardosolopes Oct 14, 2024

ghehg Oct 14, 2024 •

edited

Loading

bcardosolopes Oct 14, 2024

bcardosolopes commented Oct 17, 2024

ghehg commented Oct 18, 2024

bcardosolopes left a comment

[CIR][CIRGen] Use Clang Codegen's skeleton in CIRGenFunction::buildBuiltinExpr #967

[CIR][CIRGen] Use Clang Codegen's skeleton in CIRGenFunction::buildBuiltinExpr #967

Conversation

ghehg commented Oct 13, 2024 • edited Loading

smeenai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghehg Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardosolopes commented Oct 17, 2024

ghehg commented Oct 18, 2024

bcardosolopes left a comment

Choose a reason for hiding this comment

ghehg commented Oct 13, 2024 •

edited

Loading

ghehg Oct 14, 2024 •

edited

Loading