Skip to content

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

Merged
merged 8 commits into from
Feb 6, 2020

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Feb 5, 2020

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:

void foo(A* a) {
  int x = a->f;
  int y = a->f;
}

@MathiasVP MathiasVP added the C++ label Feb 5, 2020
Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked through the changes to the existing tests, and they look fantastic! I haven't looked through the new tests.

@dbartol or @rdmarsh2, can you vouch for the soundness of these changes?

ValueNumber operand
) {
instr.getEnclosingIRFunction() = irFunc and
instr.getResultIRType() = type and
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

@dbartol
Copy link
Contributor

dbartol commented Feb 5, 2020

This PR is great. This is exactly how I'd expect this to be implemented.

@MathiasVP MathiasVP marked this pull request as ready for review February 5, 2020 22:16
@MathiasVP MathiasVP requested review from a team as code owners February 5, 2020 22:16
instr.getEnclosingIRFunction() = irFunc and
instr.getResultIRType() = type and
valueNumber(instr.getAnOperand().(MemoryOperand).getAnyDef()) = memOperand and
valueNumberOfOperand(instr.getAnOperand().(AddressOperand)) = operand
Copy link
Contributor

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 LoadInstructions, but the implementation includes all CopyInstructions.

  • The CopyValueInstruction doesn't have an AddressOperand, so it'll be missing from this predicate even though it's present in numberableInstruction, 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 since CopyValueInstructions read from register operands rather than memory operands, and so they must always exactly overlap.
  • The StoreInstruction has an AddressOperand, but it's used for a different purpose. It doesn't have a MemoryOperand, though, so it'll be missing from this predicate. That's okay in practice for the same reasons as CopyValueInstruction.
  • The QLDoc on LoadOperand say that this operand type is also used on ReturnValueInstruction and ThrowValueInstruction. 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 separate LoadInstruction.
  • A ReadSideEffectInstruction is not a CopyInstruction, but I think it behaves like a LoadInstruction: 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, a ReadSideEffectInstruction 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 LoadInstructions only and rename it accordingly.

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants