Page MenuHomePhabricator

[Red Flag?] Generating invariant_start/end intrinsics.
Needs ReviewPublic

Authored by lvoufo on Sep 21 2015, 12:56 PM.

Details

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?

Diff Detail

Event Timeline

lvoufo updated this revision to Diff 35297.Sep 21 2015, 12:56 PM
lvoufo retitled this revision from to [Red Flag?] Generating invariant_start/end intrinsics..
lvoufo updated this object.
lvoufo added reviewers: chandlerc, majnemer, dberlin.
lvoufo updated this revision to Diff 35775.Sep 25 2015, 3:48 PM

Add test cases...

lvoufo updated this revision to Diff 35807.Sep 26 2015, 12:50 PM

This is pretty much it.
Next Steps:

  • update design doc, and
  • submit this for revision upstream.
lvoufo updated this revision to Diff 36283.Oct 1 2015, 12:45 PM

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

lvoufo updated this revision to Diff 36284.Oct 1 2015, 12:47 PM

Update diff formatting.

lvoufo updated this revision to Diff 36429.Oct 2 2015, 8:12 PM
lvoufo updated this object.

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 added a subscriber: hans.Oct 8 2015, 10:49 AM

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 {

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 added inline comments.Oct 8 2015, 2:19 PM
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 added a comment.Oct 8 2015, 3:40 PM

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 marked 20 inline comments as done.Oct 9 2015, 2:13 PM

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.