Hi Peter and Marshall,

In D60472#1461351, @peter.smith wrote:

In D60472#1461351, @peter.smith wrote:

The motivation for this change is to make "crypto" setting an additive option e.g. like "-mavx" used in many media packages. Some packages in Chrome want to enable crypto conditionally for a few files to allow crypto feature to be used based on runtime cpu detection. They set "-march=armv8+crypto" flag but it gets overridden by the global "-march=armv8a" flag set by the build system in Chrome OS because the target cpu does not support crypto causing compile-time errors.
Ability to specify "-mcrypto" standalone makes it an additive option and ensures that it it is not lost. i.e. this will help in decoupling "-mcrypto" from "-march" so that they could be set independently. The current additive alternate is '-Xclang -target-feature -Xclang "+crypto" ' which is ugly.

In D53608#1423692, @LuoYuanke wrote:

Hi
What's the status for __float128 support? Has it already been finished?

In D54487#1298688, @JohnReagan wrote:

Yes, but that is somewhat different. The "-g" option seems to just append the command line to the producer string, yes? It is harder to parse out later and it can be stripped out. gcc has both -g and -f forms.

Doesn't clang already has -grecord-gcc-switches option (https://clang.llvm.org/docs/ClangCommandLineReference.html#debug-information-flags) ?

Took another look and seems like long double is hardcoded in many of the builtins. So I think the current patch needs to rename a lot of places using long double to __float128 type.

I am also getting these tests failures because of missing trunctfxf2 and extendxftf2. These are provided by libgcc but compiler-rt does not seem to have an implementation for them.

Added checked for defined(FLOAT128) || defined(SIZEOF_FLOAT128)

lgtm. But someone more familiar with these code paths should approve.

As per https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80868, I thought GCC also emits this error but only with -pedantic. So probably should keep this error but under -Wextra or another appropriate group?

In D51713#1232497, @srhines wrote:

In D51713#1232414, @manojgupta wrote:

What is the call generated with -pg for AMR32, gnu_mcount_nc or _mount? gnu_mcount_nc with "-pg" is known to be broken ( https://bugs.llvm.org/show_bug.cgi?id=33845)

I CCed myself on that issue as we are trying to do a better job of supporting code coverage, but I don't think this bug is relevant for this patch.

Unless there are any additional reviews, I will likely submit this patch in a few hours.

What is the call generated with -pg for AMR32, gnu_mcount_nc or _mount? gnu_mcount_nc with "-pg" is known to be broken ( https://bugs.llvm.org/show_bug.cgi?id=33845)

In D51440#1217910, @mstorsjo wrote:

In D51440#1217839, @manojgupta wrote:

Just a minor comment regarding test cases: Since you are adding both -L/path/ and -l<libname>, the test cases should be updated to check for the -L/path/ argument as well.

I guess I could do that, although we don't know the path in the test, so we can only check for -L.*.

Just a minor comment regarding test cases: Since you are adding both -L/path/ and -l<libname>, the test cases should be updated to check for the -L/path/ argument as well.

In D50000#1180760, @lebedev.ri wrote:

Congrats on D50000 :)

Random uneducated thought - how does it work with multiple translation units, where not all of them have/don't have that flag set?

In D47895#1175164, @a.elovikov wrote:

Note that, I am expecting that functions with alwaysinline attribute should still get inlined. If that happens and caller does not have this attribute, then optimizer is free to remove the checks.

Not that I actually hit that, but it was questionable when reading the code - why that way? From LangRef: alwaysinline "indicates that the inliner should attempt to inline this function into callers whenever possible, ignoring any active inlining size threshold for this caller." So there is some freedom by not doing the inlining, even in the presence of the attribute.

Wouldn't it be better not to inline or to add the attribute to the caller?

Thanks for the quick review!

Thanks Piotr for the testcase.

What clang version is that? Works fine for me.

@efriedma Maybe not relevant to the patch here but our kernel devs were looking into preserve_all but it does not seem to work for AArch64.

Added helper text, updated tests.

Ping!

Thanks Petr, I can confirm that this fixed the compiler-rt build issues in Chrome OS builds.

@efriedma @jyknight Does the change match your expectations where warnings are still generated but codeGen does not emit nonnull attribute?

Do not generate calls with "nonnull" attribute with "-fno-delete-null-pointer-checks".
The warnings are still generated when nullptr is passed to a function with nonnull
attribute.

Thanks everyone, specially Eli for the reviews.

Made suggested changes.

Handle getObjectSize by passing the ObjectSizeOpts in callers.

In D47895#1150325, @george.burgess.iv wrote:

Swooping in on this admittedly quite late: should the objectsize bits in MemoryBuiltins.cpp be made aware of this?

In particular, the ObjectSizeOffsetVisitor will happily hand you "0 bytes are accessible here" if you hand it null (...though not if you ask it via __builtin_object_size, since that has a gcc-compat hack around NULL, as well). We should probably make the NullIsUnknownSize field we pass into that factor in whether the current Function has this new null-is-ok attribute on it.

...Since it's possible for us to just hand it a ConstantPointerNull without a Function attached, we probably need to plumb this in from callers of llvm::getObjectSize/llvm::lowerObjectSizeCall/...

Rebase to master + handle constant folding of intrinsic strip_invariant_group.

Replace GlobalVariable by global variable.

Updated LangRef and added comment in ConstantFolding about inability
to query the attribute.

Fix an issue pointed by efriedma. All tests still pass.

@efriedma : can you take a look again?
Thanks!

Fixed typo in inlining unit test.

In D47895#1139800, @sanjoy wrote:

Drive by comments

You also need to change the inliner to not inline null-pointer-is-dereferenceable into functions that don't have that attribute.

In D48588#1143805, @efriedma wrote:

LGTM

Made requested changes and added more tests.

@efriedma I think I have addressed the changes you requested, can you ptal?

Added a comment in GlobalOpt.cpp

In D47895#1133937, @efriedma wrote:

I went through those files but many of these addressSpace() == 0 (and != 0) checks are not for null pointers but for disabling/skipping some memory optimizations. And it is not clear to me if moving all of those checks to the new NullPointerIsDefined function makes sense there.

We should probably add a section to LangRef on address spaces, and what transforms are specifically allowed only in address space 0.

I'm sure we need to handle null-pointer-is-valid in CallSite.h, at least.

Update LangRef.

Added checks to CallSite.h and LoopAccessAnalysis.cpp.
Added another test case for atomic loads.
Moved The NullPointerIsDefined function to Function.h.

Added more test cases, updated LangRef, added a check for address space 0
in GlobalOpt.

In D47895#1131374, @efriedma wrote:

Tests look good, at first glance.

For LangRef, I was referring to https://llvm.org/docs/LangRef.html#function-attributes, yes. (There might be some gaps in the documentation, but ideally that should cover all the function attributes recognized by LLVM transforms and code generation.)

In D47895#1125388, @efriedma wrote:

Missing LangRef changes.

Updated some passes and added more test cases.

In D47895#1125388, @efriedma wrote:

Missing LangRef changes.

How did you come up with the set of checks which need to be updated? Quickly grepping through the source, I found you missed at least lib/Analysis/ValueTracking.cpp, include/llvm/IR/CallSite.h, lib/Analysis/LazyValueInfo.cpp, and lib/Analysis/ConstantFolding.cpp .

In D47894#1125406, @efriedma wrote:

Does IR generation need any special handling for this? We add nonnull attributes in various places.

Marking as approved since no comments/concerns have been raised, and change is pretty simple.

Thanks Peter.

Thanks!

Updated test case for error msg with arguments.
Updated the documentation.

Added docs, Sema test case for the attribute.

I can't reproduce this using the trunk llvm. Does it pass for you if %clang is replaced by %clang++ in the test case?