tejohnson added inline comments.
================ Comment at: clang/test/CodeGen/libcalls-fno-builtin.c:163 -// CHECK: [[ATTR]] = { nobuiltin } +// GLOBAL: #2 = { nobuiltin "no-builtins" } +// INDIVIDUAL: #2 = { nobuiltin "no-builtin-ceil" "no-builtin-copysign" "no-builtin-cos" "no-builtin-fabs" "no-builtin-floor" "no-builtin-fopen" "no-builtin-fread" "no-builtin-fwrite" "no-builtin-stpcpy" "no-builtin-strcat" "no-builtin-strchr" "no-builtin-strcmp" "no-builtin-strcpy" "no-builtin-strlen" "no-builtin-strncat" "no-builtin-strncmp" "no-builtin-strncpy" "no-builtin-strpbrk" "no-builtin-strrchr" "no-builtin-strspn" "no-builtin-strtod" "no-builtin-strtof" "no-builtin-strtol" "no-builtin-strtold" "no-builtin-strtoll" "no-builtin-strtoul" "no-builtin-strtoull" } ---------------- gchatelet wrote: > tejohnson wrote: > > Orthogonal to this patch: > > > > Looks like there are 2 nobuiltin attributes now? AFAICT the old "nobuiltin" > > gets applied to any and all cases where either -fno-builtin or > > -fno-builtin-{name} applied. Is it obviated by the new attributes? > > > > Also, why are the new ones quoted and the old one not? > Here is my understanding so far: > There are two systems for attributes. > - In the original design attributes were boolean only and they would be > packed in 64 bit integer. `nobuiltin` is one of these. > - Then the attribute system got extended to allow for more than 64 entries > and also allow for types. Attributes are now key/value pairs where value can > be bool, int or string. `"nobuiltins"` is a such boolean attribute (note the > trailing `s`) > > Now I wanted to create a different attribute than the original `nobuiltin` > because semantic might be different. > > Obviously this is not ideal. > There are two options AFAICT: > - conflate the two and reuse the original one to get a mode compact > representation > - remove the original one and release one bit for other important attribute > to use. > > For the individual ones the list is open ended so there's no way we can > retrofit it into the previous scheme. > > @efriedma, @hfinkel do you have any suggestions or a better approach here? Thanks for the background. Yeah it seems like a good idea to remove the overlap here at some point, and the semantics do seem to be different at the moment. Like I said earlier, though, I don't think it should block this particular patch since the duplication predates it (and it also allows forward progress on the TLI side). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71193/new/ https://reviews.llvm.org/D71193 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits