aaron.ballman added a reviewer: echristo. aaron.ballman added a subscriber: echristo. aaron.ballman added a comment.
Adding @echristo for the debugging info question. In D84602#2200274 <https://reviews.llvm.org/D84602#2200274>, @atrosinenko wrote: > I suspect there might be some terminology clash, so clarifying a bit just in > case. Thank you for the explanations, they help! > It was probably better to refer to these functions as //LibCalls// - > functions from the compiler support library (such as `libgcc`) such as > `__adddf3`. While there are `__popcount[sdt]i2` among them, most of the times > they are implicitly called when the high-level code performs division on > `__uint128`, for example. Unfortunately, they reside under > `compiler-rt/lib/builtins` - so that name was used... :) So, if I get it > right, you have proposed something like > `clang/include/clang/Basic/BuiltinsMips.def` and those are another kind of > builtins. Ah! That explains my confusion -- I was indeed thinking of something like BuiltinMips.def and hadn't realized this was a different kind of builtin. > The `CallingConv::MSP430_BUILTIN` was the name of a calling convention that > the MSP430 codegen of LLVM had to use //for a small subset of those support > functions//, as defined by MSP430 EABI. While it can be useful to add some > other CC for those functions for internal use by compiler-rt someday, now > there are only two CCs defined for MSP430 LibCalls: that "special convention" > for 13 functions only with two 64-bit arguments (including 64-bit integer > shifts) and the `CallingConv::C` for everything else both from the support > library and regular object files. > > Technically, these functions could probably be listed by names somewhere in > the backend code, completely detangling the upstreaming of MSP430 compiler-rt > port from D84602 <https://reviews.llvm.org/D84602> (this one), D84605 > <https://reviews.llvm.org/D84605> and, the most scary of them, D84636 > <https://reviews.llvm.org/D84636>. That may probably look a bit hackish, > though. Given that these builtins are defined in source files rather than in a .td file, I agree that would be rather hackish and using syntax would be more appropriate. ================ Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1233 return llvm::dwarf::DW_CC_LLVM_X86RegCall; + // TODO case CC_MSP430Builtin: } ---------------- aaron.ballman wrote: > atrosinenko wrote: > > aaron.ballman wrote: > > > This is likely to cause -Werror failures because the switch won't be > > > fully covered. Are you planning to do this TODO as part of this patch, or > > > in a follow up? > > > Are you planning to do this TODO as part of this patch, or in a follow up? > > > > It depends on how large that change would be. > > > > I first need to find out how such calling convention identifiers are issued > > (or maybe there already exist one from GCC). I see they are declared inside > > the `llvm/include/llvm/BinaryFormat/Dwarf.def`. > > > I'm not certain how large the change would be (and I definitely don't insist > on a full implementation), but you should ensure the switch is fully covered > so that you don't break bots when the patch lands. I still think this case needs some work to keep the bots happy. I would probably go with: ``` case CC_MSP430Builtin: // FIXME: Currently unsupported break; ``` But perhaps @echristo has opinions since this involves debugging information. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84602/new/ https://reviews.llvm.org/D84602 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
