This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add support for __builtin_memcpy_inline
ClosedPublic

Authored by gchatelet on Jan 28 2020, 4:03 AM.

Event Timeline

gchatelet created this revision.Jan 28 2020, 4:03 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 28 2020, 4:03 AM
arichardson added inline comments.Jan 28 2020, 3:06 PM
clang/docs/LanguageExtensions.rst
2259

Typo: except

clang/test/Sema/builtins-memcpy-inline.c
24

Size can only be a constant right? Should there be a test for the error reported in that case?

gchatelet updated this revision to Diff 241158.Jan 29 2020, 7:15 AM
gchatelet marked 4 inline comments as done.
  • Address comments
clang/docs/LanguageExtensions.rst
2259

Thx :)

clang/test/Sema/builtins-memcpy-inline.c
24

Thx it caught a bug indeed :)

@efriedma would you mind having a look?
It should be my last patch on that matter :)

efriedma added inline comments.Jan 30 2020, 4:23 PM
clang/lib/Sema/SemaChecking.cpp
1653

SemaBuiltinConstantArg. Or actually, you should probably use "I" in Builtins.def.

gchatelet updated this revision to Diff 241649.Jan 31 2020, 1:22 AM
gchatelet marked an inline comment as done.
  • Address comments
efriedma added inline comments.Jan 31 2020, 2:44 PM
clang/docs/LanguageExtensions.rst
2252

This is in the wrong section of the documentation. We could constant-evaluate __builtin_memcpy_inline, I guess, but that isn't the primary purpose, and your patch doesn't implement constant evaluation anyway.

Might make sense to add a new section, if no existing section makes sense.

efriedma added inline comments.Jan 31 2020, 2:45 PM
clang/lib/Sema/SemaChecking.cpp
1655

You can EvaluateKnownConstInt here, instead of isIntegerConstantExpr.

gchatelet updated this revision to Diff 241988.Feb 3 2020, 1:25 AM
gchatelet marked 3 inline comments as done.
  • Address comments
gchatelet added inline comments.Feb 3 2020, 1:28 AM
clang/docs/LanguageExtensions.rst
2252

I added a new memory builtins section. Let me know what you think.

Sorry about the delayed response, I was traveling.

clang/docs/LanguageExtensions.rst
2229

Not sure putting "memcpy" in this list makes sense. We did add support for constant-evaluating memcpy, but it was separately from the others, so the description of the feature detection is wrong.

2252

Makes sense.

gchatelet updated this revision to Diff 243289.Feb 7 2020, 2:38 PM
gchatelet marked 3 inline comments as done.
  • Remove unrelated memcpy in documentation
clang/docs/LanguageExtensions.rst
2229

Actually this is unrelated to this patch. The documentation is missing memcpy as a string builtin. It should go in a separate patch though.

efriedma accepted this revision.Feb 7 2020, 2:46 PM

LGTM

This revision is now accepted and ready to land.Feb 7 2020, 2:46 PM
This revision was automatically updated to reflect the committed changes.
melver added a subscriber: melver.Jun 17 2020, 7:23 AM
melver added inline comments.
clang/test/Sema/builtins-memcpy-inline.c
8

It appears that the expected-warning check here is guarded by the #if as well. Moving it after the #endif results in a failing test.

I noticed this as I was trying to use has_feature(builtin_memcpy_inline), but it somehow does not work, even though the compiler clearly supports __builtin_memcpy_inline.

Any idea what's wrong with the __has_feature test?

Thanks!

efriedma added inline comments.Jun 17 2020, 11:17 AM
clang/test/Sema/builtins-memcpy-inline.c
8

Should be __has_builtin, I think? __has_feature only applies to features defined in clang/include/clang/Basic/Features.def.

gchatelet marked 3 inline comments as done.Jun 18 2020, 1:58 AM
gchatelet added inline comments.
clang/test/Sema/builtins-memcpy-inline.c
8

Thx for noticing. I'll send a patch to update the test.

gchatelet marked 2 inline comments as done.Jun 18 2020, 2:03 AM
gchatelet added inline comments.
clang/test/Sema/builtins-memcpy-inline.c
8