Page MenuHomePhabricator

[clang] Annotating C++'s `operator new` with more attributes
ClosedPublic

Authored by lebedev.ri on Jan 24 2020, 1:18 PM.

Details

Summary

Right now we annotate C++'s operator new with noalias attribute,
which very much is healthy for optimizations.

However as per [[ http://eel.is/c++draft/basic.stc.dynamic.allocation | [basic.stc.dynamic.allocation] ]],
there are more promises on global operator new, namely:

  • non-std::nothrow_t operator new *never* returns nullptr
  • If std::align_val_t align parameter is taken, the pointer will also be align-aligned
  • global operator new-returned pointer is __STDCPP_DEFAULT_NEW_ALIGNMENT__-aligned It's more caveated than that.

Supplying this information may not cause immediate landslide effects
on any specific benchmarks, but it for sure will be healthy for optimizer
in the sense that the IR will better reflect the guarantees provided in the source code.

The caveat is -fno-assume-sane-operator-new, which currently prevents emitting noalias
attribute, and is automatically passed by Sanitizers (PR16386) - should it also cover these attributes?
The problem is that the flag is back-end-specific, as seen in test/Modules/explicit-build-flags.cpp.
But while it is okay to add noalias metadata in backend, we really should be adding at least
the alignment metadata to the AST, since that allows us to perform sema checks on it.

Diff Detail

Event Timeline

lebedev.ri created this revision.Jan 24 2020, 1:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2020, 1:18 PM

LLVM already infers noalias nonnull for eg. _Znwm so noalias and nonnull info added by clang will not increase power of LLVM. Or?

Alignment info is useful I think.

clang/test/CodeGenCXX/builtin-operator-new-delete.cpp
54

We lost noalias here?

lebedev.ri marked 2 inline comments as done.EditedJan 24 2020, 1:50 PM

LLVM already infers noalias nonnull for eg. _Znwm so noalias and nonnull info added by clang will not increase power of LLVM. Or?

To be honest, i don't really believe it is LLVM place to deduce attributes on standard library functions like these, frontend should be doing that.
Because as noted in patch description, if noalias is blindly "deduced" by LLVM,
that is a miscompile if -fno-assume-sane-operator-new was specificed.

Alignment info is useful I think.

That is the main motivation, yes.

clang/test/CodeGenCXX/builtin-operator-new-delete.cpp
54

See two lines above, it migrated to the callsite.

I'm going to argue that the previous behavior was erroneous:
despite what -fno-assume-sane-operator-new is supposed to be doing,
i suspect it will magically break with LTO.
(we'd have declare noalias nonnull i8* @_ZnwmSt11align_val_t(i64, i64) in one TU
and declare nonnull i8* @_ZnwmSt11align_val_t(i64, i64) in another - one with noalias
and one without - how would they be merged? by dropping noalias, or keeping it?)

rsmith added inline comments.Jan 24 2020, 7:08 PM
clang/lib/Sema/SemaDecl.cpp
14484

This should all be done by CodeGen, not by injecting source-level attributes.

14537–14555

This is incorrect. The pointer returned by operator new is only suitably aligned for any object that does not have new-extended alignment and is of the requested size. And the pointer returned by operator new[] is suitably aligned for any object that is no larger than the requested size. (These are both different from the rule for malloc, which does behave as you're suggesting here.) For example:

Suppose the default new alignment and the largest fundamental alignment are both 16, and we try to allocate 12 bytes. Then:

  • operator new need only return storage that is 4-byte aligned (because that is the largest alignment that can be required by a type T with sizeof(T) == 12)
  • operator new need only return storage that is 8-byte aligned (because that is the largest alignment that can be required by a type T with sizeof(T) <= 12)
  • malloc must return storage that is 16-byte aligned (because that is the largest fundamental alignment)
lebedev.ri marked 2 inline comments as done.Jan 25 2020, 1:52 AM
lebedev.ri added inline comments.
clang/lib/Sema/SemaDecl.cpp
14484

I don't agree, can you explain why this should be done in codegen?

14537–14555

So essentially, if we can't evaluate the requested byte count as a constant/constant range,
we must simply give up here, on the most interesting case of variable allocation size?
That is surprisingly extremely pessimizing from C++ :)

rsmith added inline comments.Jan 26 2020, 12:04 PM
clang/lib/Sema/SemaDecl.cpp
14484

Generally, it's a design goal for the Clang AST to represent the original program source (http://clang.llvm.org/docs/InternalsManual.html#faithfulness). But I suppose these attributes are all marked "implicit", and it's a good idea to express these hints uniformly to codegen and the static analyzer and so on... OK, I can make peace with doing this here.

14537–14555

The benefit from not requiring all allocations to be padded to a multiple of the largest fundamental alignment is probably worth a lot more than allowing the optimizer to assume a higher alignment for pointers from direct calls to operator new with a non-constant argument.

We can at least look for a constant argument in CodeGen and emit the alignment hint there. (Though it's probably better to do that in the middle-end, since the argument might be reduced to a constant via inlining, especially for uses of std::allocator and similar.)

lebedev.ri marked 3 inline comments as done.Jan 26 2020, 1:21 PM
lebedev.ri added inline comments.
clang/lib/Sema/SemaDecl.cpp
14484

Generally, it's a design goal for the Clang AST to represent the original program source (http://clang.llvm.org/docs/InternalsManual.html#faithfulness).

I fully agree with that goal!

But I suppose these attributes are all marked "implicit",

Yeah, this was a predefined goal here - they are added implicitly,
therefore in AST we should not pretend that they originate from source code..

and it's a good idea to express these hints uniformly to codegen and the static analyzer and so on...

Precisely.

OK, I can make peace with doing this here.

Okay.

14537–14555

Please correct me if i'm not grasping the pattern correctly:

Suppose the default new alignment and the largest fundamental alignment are both 16, and we try to allocate 12 bytes. Then:

  • operator new[] need only return storage that is 8-byte aligned (because that is the largest alignment that can be required by a type T with sizeof(T) <= 12)

Because into 12 bytes, the largest fundamental type that fits is long long int or double,
both with size/alignment of 8; and we don't care about the remaining padding, if any?

  • operator new need only return storage that is 4-byte aligned (because that is the largest alignment that can be required by a type T with sizeof(T) == 12)

"We have exactly 12 bytes. If we treat this as an array, what is the larges type that could be stored into this array, leaving no padding?"

The benefit from not requiring all allocations to be padded to a multiple of the largest fundamental alignment is probably worth a lot more than allowing the optimizer to assume a higher alignment for pointers from direct calls to operator new with a non-constant argument.

Hmm, true.

We can at least look for a constant argument in CodeGen and emit the alignment hint there.

Yes, that will be better than nothing.

(Though it's probably better to do that in the middle-end, since the argument might be reduced to a constant via inlining, especially for uses of std::allocator and similar.)

I would like to avoid doing that in middle-end, but if all else fails that will have to do.

lebedev.ri marked an inline comment as done.

For the sake of forward progress, remove overly optimistic alignment annotation, and replace it with FIXME.
I have some ideas how to deal with that, but i'd prefer to deal with all the simple stuff first.

lebedev.ri edited the summary of this revision. (Show Details)Jan 27 2020, 5:04 AM
rsmith accepted this revision.Feb 3 2020, 6:24 PM
This revision is now accepted and ready to land.Feb 3 2020, 6:24 PM

Rebased, NFC.

Rebased, NFC.

Rebased, NFC.

This revision was automatically updated to reflect the committed changes.