C++: reuse unaliased SSA results when computing aliased SSA #5522

rdmarsh2 · 2021-03-24T23:15:49Z

This PR reuses the SSA construction from the unaliased SSA IR stage when building the aliased SSA IR stage, which results in a performance speedup of about 1% across the default query suite.

jbj · 2021-04-16T13:09:25Z

It's great to see this PR moving forward!

Which CPP-Differences job is associated with this PR? https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1912/?

There are test failures around autoformatting and synced files. Two tests failed:

cpp/ql/test/query-tests/Security/CWE/CWE-114/semmle/UncontrolledProcessOperation/UncontrolledProcessOperation.qlref
cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.ql

jbj

This is not a full review. Just a copy of my review comment from #5509.

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasConfiguration.qll

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/annotate_sinks_only/tainted.expected

dbartol

Only blocking issue is the commented-out code. Otherwise looks really good, with a couple minor comments.

cpp/ql/src/semmle/code/cpp/ir/implementation/internal/TInstruction.qll

cpp/ql/src/semmle/code/cpp/ir/implementation/internal/TOperand.qll

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll

dbartol · 2021-04-16T16:16:05Z

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasedSSA.qll

@@ -131,6 +132,8 @@ abstract class MemoryLocation extends TMemoryLocation {
   * with automatic storage duration).
   */
  predicate isAlwaysAllocatedOnStack() { none() }
+
+  final predicate canReuseSSA() { any() }


This would let a hypothetical fourth IR stage reuse anything from Aliased SSA IR, right? Maybe this should be none() until we need it to be otherwise.

rdmarsh2 · 2021-05-03T21:09:13Z

I've started a new changes job: internal link

rdmarsh2 · 2021-05-05T17:22:02Z

One more job: [internal link(]https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1995/)

jbj · 2021-05-06T06:37:53Z

https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1995/ shows a speedup in SignedOverflowCheck.ql, which I think is the query that computes the whole IR 🎉. Unfortunately there's an even bigger slowdown in TaintedPath.ql, which is the query that computes the shared/cached IR data-flow predicates.

My best guess is that you've made the IR faster but that some size estimates have changed and have triggered a bad join in some code that uses the IR.

To make the next CPP-Differences fair, please merge from main into this branch. The performance improvements in #5829 could interact with your changes here.

This refactors the SSA stages of the IR so that instructions which have a modeled memory result in the unaliased SSA stage do not have SSA recomputed in the aliased SSA stage.

MathiasVP · 2021-05-19T08:05:07Z

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysis.qll

+    (
+      f.parameterEscapesOnlyViaReturn(operand.(PositionalArgumentOperand).getIndex())
+      or
+      f.parameterEscapesOnlyViaReturn(-1) and


Would it be more aesthetically pleasing to add a int getIndex() { none() } predicate to ArgumentOperand and implement it as result = -1 for ThisArgumentOperand?

jbj · 2021-05-19T09:23:42Z

I'm happy to see there's progress on this PR. Are you ready to run CPP-Differences?

rdmarsh2 · 2021-05-19T18:20:22Z

Yes. New job is here: internal link

rdmarsh2 · 2021-05-21T20:48:38Z

Rerunning the job after some overnight issues with the CI system: internal link

rdmarsh2 · 2021-05-24T21:27:19Z

Looking at those numbers, there's a range from a slowdown of 1.5% to a speedup of 4.5%, with an average. Given that most of the ones that slowed down have bigger slowdowns in Critical/NewArrayDeleteMismatch.ql, which doesn't use the IR, I think that's reflecting machine noise rather than real changes.

MathiasVP · 2021-05-25T07:42:45Z

Code and CPP-differences LGTM!
I'm guessing the three new cpp/comparison-with-wider-type results are noise since they don't use the IR.

@dbartol I think your blocking issue has been fixed, right?

jbj · 2021-05-31T06:15:03Z

The performance results look great!

I'm guessing the three new cpp/comparison-with-wider-type results are noise since they don't use the IR.

I believe those "new" alerts are the ones that were fixed in #5859. If they're showing up here, it's an indication that the base commit in the comparison is too new. Unless the IR has become slower on main recently, that should not make the performance improvement any less valid.

github-actions bot added C# C++ labels Mar 24, 2021

rdmarsh2 force-pushed the rdmarsh2/cpp/ssa-reuse branch 4 times, most recently from 26ba625 to 75920ff Compare April 14, 2021 23:14

rdmarsh2 marked this pull request as ready for review April 16, 2021 00:06

rdmarsh2 requested review from a team as code owners April 16, 2021 00:06

rdmarsh2 requested a review from dbartol April 16, 2021 00:09

jbj assigned dbartol Apr 16, 2021

jbj reviewed Apr 16, 2021

View reviewed changes

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasConfiguration.qll Outdated Show resolved Hide resolved

jbj reviewed Apr 16, 2021

View reviewed changes

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/annotate_sinks_only/tainted.expected Outdated Show resolved Hide resolved

jbj mentioned this pull request Apr 16, 2021

C++: make unaliased_ssa IR stage sound #5509

Closed

dbartol suggested changes Apr 16, 2021

View reviewed changes

MathiasVP added the no-change-note-required This PR does not need a change note label Apr 16, 2021

rdmarsh2 and others added 10 commits May 6, 2021 08:14

C++: add test for IR alias analysis soundness

d95ef89

C++: make unaliased_ssa IR stage sound

a9d7990

C++: Reuse SSA from earlier stages

deff5c3

This refactors the SSA stages of the IR so that instructions which have a modeled memory result in the unaliased SSA stage do not have SSA recomputed in the aliased SSA stage.

autoformat and sync C++ files

8bc7e59

C++: accept phi node reorderings in IR tests

1c72ea9

C++: remove points-to expectations for reused SSA

f9e0ba1

C++: accept test regressions

86b1d03

C++/C#: Add combineOverlap() predicate

922cf64

C++: tests for phi nodes after unreachable blocks

7930c4a

C++: handle degenerate phi nodes

6600436

rdmarsh2 added 12 commits May 6, 2021 08:14

C++: handle phi operands from unreachable blocks

195b811

C++: test changes in annotate_sinks_only

1f69b31

C++: fixups and file sync for SSA sharing

5d7d26b

C#: sync IR files and update for C++ SSA reuse

b281102

C++: accept test changes from IR sharing

230f4bc

C++: autoformat

5406783

C++: fix bad join order in phi node sharing

35594ea

C++: autoformat

5318aa8

C++/C#: fix another join order in SSA construction

b3e598c

C++: One more join order fix

9ac55af

C++: Handle alias models for this/qualifiers

e590a7b

C++: fix alias model for smart pointer setters

db85a21

rdmarsh2 force-pushed the rdmarsh2/cpp/ssa-reuse branch from 247a7f4 to db85a21 Compare May 18, 2021 20:16

MathiasVP reviewed May 19, 2021

View reviewed changes

dbartol approved these changes Jun 1, 2021

View reviewed changes

dbartol merged commit da14647 into main Jun 1, 2021

dbartol deleted the rdmarsh2/cpp/ssa-reuse branch June 1, 2021 14:17

MathiasVP added a commit to MathiasVP/ql that referenced this pull request Jun 9, 2021

C++: Match the join order from before github#5522.

879bfbb

MathiasVP mentioned this pull request Jun 9, 2021

C++: Fix join order in addressOperandAllocationAndOffset #6050

Merged

C++: reuse unaliased SSA results when computing aliased SSA #5522

C++: reuse unaliased SSA results when computing aliased SSA #5522

Uh oh!

Conversation

rdmarsh2 commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbj commented Apr 16, 2021

Uh oh!

jbj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dbartol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dbartol Apr 16, 2021

Choose a reason for hiding this comment

Uh oh!

rdmarsh2 commented May 3, 2021

Uh oh!

rdmarsh2 commented May 5, 2021

Uh oh!

jbj commented May 6, 2021

Uh oh!

MathiasVP May 19, 2021

Choose a reason for hiding this comment

Uh oh!

jbj commented May 19, 2021

Uh oh!

rdmarsh2 commented May 19, 2021

Uh oh!

rdmarsh2 commented May 21, 2021

Uh oh!

rdmarsh2 commented May 24, 2021

Uh oh!

MathiasVP commented May 25, 2021

Uh oh!

jbj commented May 31, 2021

Uh oh!

Uh oh!

rdmarsh2 commented Mar 24, 2021 •

edited

Loading