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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
