-
Notifications
You must be signed in to change notification settings - Fork 1.7k
C++: Better value numbering support for loading fields in IR #2772
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ValueNumber operand | ||
) { | ||
instr.getEnclosingIRFunction() = irFunc and | ||
instr.getResultIRType() = type and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there test cases where the type is required to avoid conflating values? If so, I'd be surprised if those cases can't be tweaked so the type is the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Off the top of my head:
double foo(void* p, bool b) {
if (b) {
return *(int*)p;
}
else {
return *(float*)p;
}
}
I'm not sure we'd treat the results of the two casts as congruent today, but we probably should. However, we would not want the results of the two indirect loads to be congruent, even though their address operands were congruent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I included the type mainly because all the other constructors did so. I will take a further look tomorrow and see if it actually makes a difference
cpp/ql/test/library-tests/valuenumbering/GlobalValueNumbering/ir_gvn.expected
Show resolved
Hide resolved
cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll
Show resolved
Hide resolved
This PR is great. This is exactly how I'd expect this to be implemented. |
7d7ed10
to
ba395cf
Compare
instr.getEnclosingIRFunction() = irFunc and | ||
instr.getResultIRType() = type and | ||
valueNumber(instr.getAnOperand().(MemoryOperand).getAnyDef()) = memOperand and | ||
valueNumberOfOperand(instr.getAnOperand().(AddressOperand)) = operand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What CopyInstruction
types actually have an AddressOperand
? I believe this PR is intended for LoadInstruction
s, but the implementation includes all CopyInstruction
s.
- The
CopyValueInstruction
doesn't have anAddressOperand
, so it'll be missing from this predicate even though it's present innumberableInstruction
, which means it'll have no value number at all, not even a unique one. I'm guessing that's not a problem in practice sinceCopyValueInstruction
s read from register operands rather than memory operands, and so they must always exactly overlap. - The
StoreInstruction
has anAddressOperand
, but it's used for a different purpose. It doesn't have aMemoryOperand
, though, so it'll be missing from this predicate. That's okay in practice for the same reasons asCopyValueInstruction
. - The QLDoc on
LoadOperand
say that this operand type is also used onReturnValueInstruction
andThrowValueInstruction
. Is there something to be done for those instructions too? I don't think there is, because those instructions don't return anything. Their loads are always exact, so they won't be relevant in this case. I wonder why those instructions have a load built into them instead of using a separateLoadInstruction
. - A
ReadSideEffectInstruction
is not aCopyInstruction
, but I think it behaves like aLoadInstruction
: it has an address and a memory operand, and that memory operand can be (is always?) inexact. It seems to me like we'd want value numbers for the indirections that are accessed by these read-side-effect instructions, but I don't see how it's possible. First, aReadSideEffectInstruction
does not return a result; second, we can't tell without analysing or modelling the callee what portion of the data will be loaded.
Okay, most of this discussion was just me thinking out loud and confirming that for the way the IR is generated today we're fine. But I think the right thing to do, for future robustness, is to restrict CongruentCopyInstructionTotal
to LoadInstruction
s only and rename it accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've restricted it to extend LoadInstruction
only now, and renamed it CongruentCopyInstructionTotal
for lack of a better name. The test results are identical.
…ion and extend LoadInstruction instead of CopyInstruction
This PR improves the IR value numbering library such that the two
a->f
expressions are assigned the same value number in the example below: