[PATCH] D71193: [clang] Turn -fno-builtin flag into an IR Attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet marked an inline comment as done.
gchatelet 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" }
----------------
tejohnson wrote:
> 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).
SGTM I'll submit the patch then. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71193/new/

https://reviews.llvm.org/D71193