[Red Flag?] Generating invariant_start/end intrinsics.

[lvoufo]

Summary

[Continued from D11826; sans 'writeonce' annotations.]

Ready to submit for review upstream, based on design doc review.

Please let me know if you see any red flag so far?

[lvoufo]

Add test cases...

[lvoufo]

This is pretty much it.
Next Steps:

• update design doc, and
• submit this for revision upstream.

[lvoufo]

Add test cases for mutable fields, const references and pointers and (basic) inheritance.

[lvoufo]

Update diff formatting.

[lvoufo]

Allow objects with mutable fields to be candidates for writeonce semantics; and specify writeonce candidates as trivial objects with no non-trivial method that may write to memory.

[hans]

Some drive-by comments.

include/clang/AST/Type.h

712

This will need a comment.

lib/AST/DeclCXX.cpp

1348

Maybe use an assert instead? IsWriteOnceCandidate is already initialized to false.

1367

Doesn't not defined just mean it can be defined out-of-line? Do you want M->isPure() instead?

1399

Left-overs from an old patch version?

lib/AST/Type.cpp

1942

No const-object optimization in C? :-(

1946

Would it be safer to use a white-list approach? I.e. return false by default, and explicitly check for a whitelist of things (isConstant, CXXRecord::isWriteOnceCandidate).

lib/CodeGen/CGDecl.cpp

947

ultra nit: } else {

[lvoufo]

Good catches. I'll make sure to incorporate these observations in the initial patch.

lib/AST/DeclCXX.cpp

1367

"not defined" here is implemented using "isDefined()", which also checks for out-of-line definitions. I think I probably mean M->isPure() afterall. I will have to double check that.

1399

Looks I I need some clean up.

lib/AST/Type.cpp

1942

Hmm... This is following what we are currently with global constants. (cf. QualType::isConstant())

[majnemer]

include/clang/AST/DeclCXX.h

777

This would need a comment.

lib/AST/DeclCXX.cpp

1337

isWriteOnceCandidate + the const qualifier on the method would indicate to me that this method is just a getter for IsWriteOnceCandidate but it actually computes a result and caches it. Perhaps it should be called something like computeWriteOnceCandidacy.

1350

Why can we skip ctors and dtors?

1362

Pointers lean right.

1371

You are checking if the method is virtual but I think you also need to check that the method is pure.

lib/CodeGen/CGDecl.cpp

861–864

Can you clang-format this?

862

References should lean right.

865

Is list-initialization needed here?

944

Pointers lean right. Also, consider using auto * for the declaration of CAddr.

949

Is this code supposed to be dead?

lib/CodeGen/CGDeclCXX.cpp

158–159

Please use braces around the else side.

lib/CodeGen/CodeGenFunction.h

3222

Perhaps this should have RAII in it's name to make it clear that it's destructor is stateful?

3229

References lean right.

lib/CodeGen/CodeGenModule.h

266

Please start variables with upper-case letters.

267

Please clang-format this.

[lvoufo]

Cool. Roger. Thanks!

lib/AST/DeclCXX.cpp

1350

The invariant intrinsic calls are generated right after the construction of a given const object (which must be initialized) and right before its destruction at the end of its lifetime.
Both constructors and destructors modify the allocated memory and will not be called in the middle of an invariant_start/end pair, and thus may not affect the reduction of loads. So, I don't think it helps to handle constructors here as well.

[lvoufo]

Thanks for all the comments. All should be addressed with D13603.

lib/AST/DeclCXX.cpp

1350

Added this information as comment.

lib/AST/Type.cpp

1942

I've added a TODO for you here. :)

1946

I am not exactly sure how this would be safer, but I can see how it would be more expressive. Thanks!

lib/CodeGen/CGDecl.cpp

865

I think so, for "uniform initialization" purposes.