C++: Global value numbering for function calls - #9892
Conversation
This leverages the existing alias analysis to identify functions which have no reads or writes of the AllAliasedMemory virtual variable, and therefore have no global side effects. A recursion over the call graph identifies functions which have no indirect global side effects, and calls to those functions have their global side effect instructions removed.
MathiasVP
left a comment
There was a problem hiding this comment.
The changes to .expected files LGTM! I've left a couple of comments regarding how many calls we really want to GVN.
I'm eagerly waiting for the missing SideEffectElimination file. Once that has been checked in we should definitely do a DCA run on this. If there's a sign of an unacceptable slowdown we could consider GVN'ing fewer calls like I've suggested in a couple of comments.
| ) | ||
| } | ||
|
|
||
| predicate callValueNumber(CallInstruction call, int index, TCallPartialValueNumber vn) { |
There was a problem hiding this comment.
Should we maybe exclude CallInstructions that has a void return type? That's a slightly less drastic change, and it'll suffice for our vector size use-case.
There was a problem hiding this comment.
Also, I think all of the new definitions can be marked as private, no?
There was a problem hiding this comment.
Actually the place to do this is in CallPartialValueNumber
| exists(CallSideEffectInstruction cse | | ||
| cse.getPrimaryInstruction() = call and | ||
| cse.getSideEffectOperand().getAnyDef() = instr and | ||
| argIndex = -2 and | ||
| isEffect = false | ||
| ) | ||
| or | ||
| exists(CallReadSideEffectInstruction cse | | ||
| cse.getPrimaryInstruction() = call and | ||
| cse.getSideEffectOperand().getAnyDef() = instr and | ||
| argIndex = -2 and | ||
| isEffect = false | ||
| ) | ||
| or |
There was a problem hiding this comment.
It probably doesn't matter a lot, but shouldn't both of these be isEffect = true?
There was a problem hiding this comment.
Alternatively, we could choose to only GVN functions that are side-effect free (which I think would cover std::vector's size function, right?)
There was a problem hiding this comment.
I'd prefer to GVN all functions if we can, and eventually GVN the memory side effects as well (but that's work for another PR)
| import semmle.code.cpp.ir.implementation.raw.internal.IRConstruction as IRConstruction | ||
| import semmle.code.cpp.ir.implementation.unaliased_ssa.internal.SSAConstruction as UnaliasedSsa | ||
| import semmle.code.cpp.ir.implementation.aliased_ssa.internal.SSAConstruction as AliasedSSA | ||
| import semmle.code.cpp.ir.implementation.aliased_ssa.internal.SideEffectElimination as Elim |
There was a problem hiding this comment.
I don't think you've checked SideEffectElimination.qll in yet. That explains all the consistency errors in ql-for-ql.
| } | ||
|
|
||
| predicate callPartialValueNumber(CallInstruction call, int index, TCallPartialValueNumber head) { | ||
| index = 1 and head = TNilArgument() |
There was a problem hiding this comment.
I would really prefer this to be index = 0. Is there a reason it has to be index = 1?
There was a problem hiding this comment.
... Apparently I got my recursion wrong here - this is actually skipping the first item in the rank.
| call.getStaticCallTarget() = callee and | ||
| call.getEnclosingFunction() = func | ||
| | | ||
| noTransitiveSideEffectWrite(callee) |
There was a problem hiding this comment.
I'm always scared of recursion through forall as it's very easy to get a quadratic blowup. There's a general performance trick to this described in this internal issue: https://github.com/github/codeql-core/issues/416. We might want to apply that here.
|
Small update on this PR. There's a very large performance regression. I would have guessed that it was due to the recursion through |
eafdc28 to
09d6ea2
Compare
09d6ea2 to
9ddacf5
Compare
This PR adds global value numbering for function calls, taking into account side effects. It also eliminates global side effects for function calls when the function does not use or modify non-local state other than what's immediately pointed to by its parameters.