Page MenuHomePhabricator

Implement strip.invariant.group
ClosedPublic

Authored by Prazek on May 19 2018, 6:53 AM.

Details

Summary

This patch introduce new intrinsic -
strip.invariant.group that was described in the
RFC: Devirtualization v2

Diff Detail

Repository
rL LLVM

Event Timeline

Prazek created this revision.May 19 2018, 6:53 AM
Prazek updated this revision to Diff 147661.May 19 2018, 7:08 AM
  • Add deleted tests
Prazek updated this revision to Diff 147663.May 19 2018, 7:21 AM
  • small fixes in tests
Prazek updated this revision to Diff 147665.May 19 2018, 8:04 AM

Reformat

Prazek updated this revision to Diff 147683.May 19 2018, 3:11 PM

introduced mayBeDynamicClass and added more tests

xbolva00 added inline comments.May 19 2018, 3:22 PM
clang/include/clang/AST/DeclCXX.h
778 ↗(On Diff #147683)
Prazek updated this revision to Diff 147684.May 19 2018, 3:55 PM

Changed comments

Prazek updated this revision to Diff 147688.May 19 2018, 4:47 PM

After rebasing

The changes to Clang generally seem reasonable, but I think you should split them into a separate commit from the commit that adds the intrinsic itself.

clang/lib/CodeGen/CGExpr.cpp
3858 ↗(On Diff #147691)

Please add a comment explaining why this is necessary. (I'm actually not sure why it is, because surely the invariant groups we generate don't contain assumptions about memory from fields, right?)

clang/lib/CodeGen/CGExprScalar.cpp
1626 ↗(On Diff #147691)

Again, comment.

1767 ↗(On Diff #147691)

I think assigning to IntToPtr would be clearer here. You're changing the value returned, not introducing a reason to early-exit.

And a comment, please, on all of these cases.

Prazek updated this revision to Diff 148313.May 23 2018, 4:15 PM
Prazek marked 2 inline comments as done.

Slitted commit into defining and using intrinsic

clang/lib/CodeGen/CGExpr.cpp
3858 ↗(On Diff #147691)

Short answer: you can only make virtual calls on a dynamic pointer that carries invariant.group and you can't do anything other because it could leak the information about this pointer (which when used with comparison could break devirtualization).

rjmccall added inline comments.May 23 2018, 9:08 PM
clang/lib/CodeGen/CGExpr.cpp
3858 ↗(On Diff #147691)

Alright, sure.

Prazek added a comment.Jun 2 2018, 1:45 PM

friendly ping

Prazek edited reviewers, added: davide; removed: rjmccall.Jun 13 2018, 6:43 AM
rsmith accepted this revision.Jun 28 2018, 2:37 PM
rsmith added inline comments.
llvm/docs/LangRef.rst
12928 ↗(On Diff #148313)

does carries -> carries

This revision is now accepted and ready to land.Jun 28 2018, 2:37 PM
kuhar accepted this revision.Jul 1 2018, 3:47 PM
Closed by commit rL336073: Implement strip.invariant.group (authored by Prazek, committed by ). · Explain WhyJul 1 2018, 9:54 PM
This revision was automatically updated to reflect the committed changes.
Prazek marked an inline comment as done.