Page MenuHomePhabricator

[clang] Turn -fno-builtin flag into an IR Attribute
ClosedPublic

Authored by gchatelet on Dec 9 2019, 3:30 AM.

Details

Summary

This is a follow up on https://reviews.llvm.org/D61634#1742154 to turn the clang driver -fno-builtin flag into an IR attribute.
I also investigated pushing the attribute earlier on (in Sema) but it looks like this patch is simple and will cover all function calls.

Diff Detail

Event Timeline

gchatelet created this revision.Dec 9 2019, 3:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2019, 3:30 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Adding a few more reviewers who may know more about LLVM attributes.

clang/lib/CodeGen/CGCall.cpp
1885

You can drop the top-level const here (we don't typically top-level const qualify locals).

1891–1892

llvm::for_each()?

1894–1895

llvm::for_each()?

gchatelet updated this revision to Diff 233272.Dec 11 2019, 1:12 AM
gchatelet marked 3 inline comments as done.
  • Address comments

This LGTM, although I have a couple of questions that are orthogonal to this patch and shouldn't block it. Please wait to see if @aaron.ballman has any more comments.

clang/test/CodeGen/libcalls-fno-builtin.c
163–164

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?

gchatelet marked an inline comment as done.Dec 12 2019, 2:32 AM
gchatelet added inline comments.
clang/test/CodeGen/libcalls-fno-builtin.c
163–164

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?

tejohnson added inline comments.Dec 12 2019, 7:35 AM
clang/test/CodeGen/libcalls-fno-builtin.c
163–164

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).

gchatelet marked an inline comment as done.Dec 12 2019, 7:54 AM
gchatelet added inline comments.
clang/test/CodeGen/libcalls-fno-builtin.c
163–164

SGTM I'll submit the patch then.

tejohnson accepted this revision.Dec 12 2019, 7:58 AM
This revision is now accepted and ready to land.Dec 12 2019, 7:58 AM
This revision was automatically updated to reflect the committed changes.