Enable perfect forwarding with irept/exprt/typet constructors #3502

tautschnig · 2018-12-02T11:19:15Z

This should enable use (with performance benefit) of rvalue references in
higher-level APIs.

Each commit message has a non-empty body, explaining why the change was made.
Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
n/a The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
n/a My commit message includes data points confirming performance improvements (if claimed).
My PR is restricted to a single feature or bugfix.
n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.

peterschrammel

n/a My commit message includes data points confirming performance improvements (if claimed)

Isn't your commit message claiming exactly that? ;)

tautschnig · 2018-12-02T12:26:41Z

Isn't your commit message claiming exactly that? ;)

No, I'm not yet claiming that: I'm working on a PR that will actually start making serious use of these functions, at which point I should likely do a proper evaluation.

tautschnig · 2018-12-02T12:56:02Z

Although I have a fix for the test failures I'm marking this do-not-merge as I'll add the corresponding changes to *typet/*exprt/code*t in a single branch to do performance benchmarking. (Also, the failing tests did confirm that the functions are used.)

src/util/irep.cpp

tautschnig · 2018-12-02T15:40:13Z

Rvalue constructors added. I will now start testing and benchmarking. Various preparatory changes have been factored out into other PRs, and there is a lot that could and should be done on top of this one.

smowton · 2018-12-03T09:59:25Z

src/util/irep.cpp

+
+  return it->second;
+#else
+  named_subt::iterator entry = s.find(name);


How about just s[name] = std::move(irep);?

We also want to return a reference to the newly added irept.

smowton · 2018-12-03T10:01:12Z

src/util/std_code.h

@@ -479,6 +494,11 @@ class code_assumet:public codet
    add_to_operands(expr);
  }

+  explicit code_assumet(exprt &&expr):codet(ID_assume)


Style: spacing of : (here and below)

smowton · 2018-12-03T10:14:01Z

Paging @NathanJPhillips for C++ wizardry: If you have a function with overloads f(const xyz &) and f(xyz &&), and a function that takes a few xyzs and applies f to each one, is there a good way to get optimal use of f? For example, in this PR we have something like

g(const xyz &a, const xyz &b, const xyz &c)
{
  f(a); f(b); f(c);
}

g(xyz &&a, xyz &&b, xyz &&c)
{
  f(std::move(a)); f(std::move(b)); f(std::move(c));
}

But, (a) it's annoying to duplicate every method like g, which here means all the constructors on exprt children, and (b) this doesn't catch the case where a, b and c are mixed lvalue and rvalue refs.

I'd like a case like

template<class A, class B, class C>
g(A &&a, B &&b, C &&c)
{
  f(std::forward(a)); f(std::forward(b)); f(std::forward(c));
}

But it seems odd to have A, B and C be that general when actually I only want to allow const xyz & or xyz &&. Is there a nice way to do this?

allredj

⚠️
This PR failed Diffblue compatibility checks (cbmc commit: 8cc4c5e).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/103558660
Status will be re-evaluated on next push.
Common spurious failures:

the cbmc commit has disappeared in the mean time (e.g. in a force-push)
the author is not in the list of contributors (e.g. first-time contributors).
the compatibility was already broken by an earlier merge.

kroening · 2019-03-14T14:56:33Z

src/util/ssa_expr.h

@@ -18,7 +18,7 @@ class ssa_exprt:public symbol_exprt
 public:
  /// Constructor
  /// \param expr: Expression to be converted to SSA symbol
-  explicit ssa_exprt(const exprt &expr) : symbol_exprt(irep_idt(), expr.type())
+  explicit ssa_exprt(const exprt &expr) : symbol_exprt(expr.type())
  {


I kind of liked that symbol_exprt now enforces that you set the identifier.

I don't think symbol_exprt actually does? This change here just removes an unnecessary call to set_identifier.

No, it doesn't, but I was hoping it would eventually.

We can certainly put the irep_idt() back if and when we do.

kroening · 2019-03-14T15:00:29Z

src/util/std_code.h

  {
-    arguments() = _arguments;
+    arguments() = std::move(_arguments);
  }


You could make the above multi_ary_exprt(ID_arguments, std::move(_arguments))

These do not actually destroy the source object, but ensure that using dstringt with std::move is as efficient as a regular copy/assignment.

This should enable use (with performance benefit) of rvalue references in higher-level APIs.

Uses the recently added irept constructor to move all arguments in place.

Also don't use subtype() in the constructor as that would unnecessarily construct a blank irept first.

allredj

⚠️
This PR failed Diffblue compatibility checks (cbmc commit: 4b20ea0).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/104425235
Status will be re-evaluated on next push.
Common spurious failures:

the cbmc commit has disappeared in the mean time (e.g. in a force-push)
the author is not in the list of contributors (e.g. first-time contributors).
the compatibility was already broken by an earlier merge.

kroening · 2019-03-14T15:07:57Z

src/util/std_expr.h

  {
-    PRECONDITION(op.type().id()==ID_bool);
+    PRECONDITION(op().type().id() == ID_bool);
  }


~~Hmm, but then I should really add as_const() in there ;-)~~ Edit: Done.

Unfortunately, this doesn't do the job -- the mere access to operands() is enough to wreck the sharing.
This needs to be as_const(*this).type().id().

My apologies for failing to take into account this remark right here, the fix is now in #4391.

Profiling does not show any advantage of using rvalue constructors over passing by value (and then moving in place), and this requires at most as many copies as the previous const-reference case. Also make use of the new exprt constructors to set up operands without an additional add_to_operands call.

Profiling does not show any advantage of using rvalue constructors over passing by value (and then moving in place), and this requires at most as many copies as the previous const-reference case.

allredj

⚠️
This PR failed Diffblue compatibility checks (cbmc commit: 0520791).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/104426210
Status will be re-evaluated on next push.
Common spurious failures:

the cbmc commit has disappeared in the mean time (e.g. in a force-push)
the author is not in the list of contributors (e.g. first-time contributors).
the compatibility was already broken by an earlier merge.

Profiling does not show any advantage of using rvalue constructors over passing by value (and then moving in place), and this requires at most as many copies as the previous const-reference case. Also make use of the new exprt constructors to set up operands without an additional add_to_operands call.

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 83ed9a5).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/104448810

tautschnig requested review from kroening, peterschrammel, pkesseli and smowton as code owners December 2, 2018 11:19

peterschrammel reviewed Dec 2, 2018

View reviewed changes

tautschnig added the do not merge label Dec 2, 2018

kroening reviewed Dec 2, 2018

View reviewed changes

src/util/irep.cpp Show resolved Hide resolved

tautschnig force-pushed the irept-rvalue-add-set branch from e7c4998 to 0960485 Compare December 2, 2018 15:37

tautschnig force-pushed the irept-rvalue-add-set branch from 0960485 to f2878f2 Compare December 2, 2018 19:26

tautschnig requested review from cesaro, chrisr-diffblue, cristina-david, martin-cs, NathanJPhillips and thk123 as code owners December 2, 2018 19:26

smowton reviewed Dec 3, 2018

View reviewed changes

tautschnig self-assigned this Dec 17, 2018

tautschnig force-pushed the irept-rvalue-add-set branch from f2878f2 to a22b7c4 Compare December 19, 2018 23:01

tautschnig force-pushed the irept-rvalue-add-set branch 2 times, most recently from d757405 to 2649b30 Compare December 28, 2018 19:40

tautschnig force-pushed the irept-rvalue-add-set branch 4 times, most recently from 10286d6 to 1ddd740 Compare January 5, 2019 19:34

tautschnig force-pushed the irept-rvalue-add-set branch 2 times, most recently from 0b0b19b to 9f0bdad Compare January 15, 2019 18:14

tautschnig force-pushed the irept-rvalue-add-set branch 2 times, most recently from 5547be1 to f0dc891 Compare March 7, 2019 10:27

tautschnig mentioned this pull request Mar 7, 2019

Simplify the interface to goto_symex_statet::add_object #4342

Merged

3 tasks

tautschnig force-pushed the irept-rvalue-add-set branch from f0dc891 to 8cc4c5e Compare March 7, 2019 14:16

allredj reviewed Mar 7, 2019

View reviewed changes

tautschnig mentioned this pull request Mar 8, 2019

Move make_allocate_code utility function #4349

Merged

2 tasks

tautschnig force-pushed the irept-rvalue-add-set branch 2 times, most recently from 4b20ea0 to 0520791 Compare March 14, 2019 13:31

kroening reviewed Mar 14, 2019

View reviewed changes

tautschnig added 4 commits March 14, 2019 15:03

Move constructor/assignment for dstringt

c61a3ba

These do not actually destroy the source object, but ensure that using dstringt with std::move is as efficient as a regular copy/assignment.

Pass irept by value in modifying irept::add/set operations

027760b

This should enable use (with performance benefit) of rvalue references in higher-level APIs.

Add pass-by-value constructor to exprt

60e9284

Uses the recently added irept constructor to move all arguments in place.

Pass-by-value constructors for typet and type_with_subtypet

c7d38df

Also don't use subtype() in the constructor as that would unnecessarily construct a blank irept first.

allredj reviewed Mar 14, 2019

View reviewed changes

kroening reviewed Mar 14, 2019

View reviewed changes

tautschnig added 2 commits March 14, 2019 15:11

Pass-by-value in *typet constructors

3924933

Profiling does not show any advantage of using rvalue constructors over passing by value (and then moving in place), and this requires at most as many copies as the previous const-reference case.

kroening approved these changes Mar 14, 2019

View reviewed changes

tautschnig force-pushed the irept-rvalue-add-set branch from 0520791 to 8e1191f Compare March 14, 2019 15:15

allredj reviewed Mar 14, 2019

View reviewed changes

tautschnig force-pushed the irept-rvalue-add-set branch from 8e1191f to 83ed9a5 Compare March 14, 2019 15:29

allredj reviewed Mar 14, 2019

View reviewed changes

kroening assigned tautschnig and unassigned smowton, kroening and peterschrammel Mar 14, 2019

tautschnig merged commit 8334ac8 into diffblue:develop Mar 14, 2019

tautschnig deleted the irept-rvalue-add-set branch March 14, 2019 20:31

Enable perfect forwarding with irept/exprt/typet constructors #3502

Enable perfect forwarding with irept/exprt/typet constructors #3502

Uh oh!

Conversation

tautschnig commented Dec 2, 2018

Uh oh!

peterschrammel left a comment

Choose a reason for hiding this comment

Uh oh!

tautschnig commented Dec 2, 2018

Uh oh!

tautschnig commented Dec 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tautschnig commented Dec 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smowton commented Dec 3, 2018

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tautschnig Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tautschnig commented Dec 2, 2018 •

edited

Loading

tautschnig Mar 14, 2019 •

edited

Loading