yaohuihan-iluvatar wrote: > Overall I think the implementation of this pr per se is mostly fine (modulo > the question of where that helper function should live), but this situation > as a whole also seems a bit... terrible candidly speaking: having to remember > to ‘resolve’ builtin IDs like this whenever we want to query what builtin a > certain function is feels like a questionable design choice to me, but that > said I'm also not terribly familiar w/ auxiliary builtins... is there > anything preventing us from making it so builtin IDs just never overlap? I > doubt we're running out of integers... > > CC @AaronBallman Not sure who the maintainer for this is, but I wonder if > there's a reason why the builtin ids couldn't just not overlap?
Your understanding is essentially correct — the IDs do overlap. But a couple of points worth adding: The overlap is by construction, not because we're short on integers: each target's TS-builtin enum is generated independently from its own .def and is numbered starting at the same FirstTSBuiltin base, so X86::BI* and ARM::BI* occupy the same integer range. In a normal single-target compile only one target's builtin table is registered, so IDs are actually unique within a TU; **the overlap only becomes observable with an aux target (OpenMP/CUDA/HIP offload), which is the only situation where two target builtin tables coexist.** The bigger issue is that making IDs globally unique wouldn't actually remove the "resolve" step — it would just move it. **All the per-target handlers (EmitX86BuiltinExpr, the constexpr switch (case X86::BI...), etc.) are written in target-local numbering, so getAuxBuiltinID isn't only about de-duplication; it translates the runtime ID back into the target's own enum value that those switches expect.** With globally-unique IDs you'd still need an ID -> (target, local ID) mapping, or you'd have to regenerate every target's switch to use global IDs. Also, the un-normalized ID deliberately encodes the "primary vs aux" bit, and dispatch code relies on it — e.g. **EmitTargetBuiltinExpr checks isAuxBuiltinID precisely so it can pick the aux target's triple/arch.** So we can't normalize once-and-early without losing the information of which target the builtin came from. Given all that, I agree the "remember to resolve everywhere" pattern is a footgun **(this very bug is proof — CodeGen and Sema resolve, the constexpr path didn't)**. https://github.com/llvm/llvm-project/pull/201805 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
