C++: Change range-analysis test to not use getAst

[MathiasVP]

For a while we were confused about this range-analysis test:

void* f3_get(int n);
void f3() {
  int n = 0;
  while (f3_get(n)) n+=2;

  for (int i = 0; i < n; i += 2) {
    range(i); // $ range=>=0 SPURIOUS: range="<=call to f3_get-1" range="<=call to f3_get-2"
  }
}

because the inline-expectations test annotation seemed to suggest that range-analysis was claiming that i was bounded upper bounded by the return value of the call to f3_get (which totally didn't make any sense). However, as @jketema correctly hypothesized, this was actually caused by the fact that we were using Instruction.getAst to obtain a string-representation of the bound. Dropping the use of getAst (as I've done in PR) reveals what's really going on:

void* f3_get(int n);
void f3() {
  int n = 0;
  while (f3_get(n)) n+=2;

  for (int i = 0; i < n; i += 2) {
    range(i); // $ range=>=0 SPURIOUS: range="<=Phi: call to f3_get-1" range="<=Phi: call to f3_get-2"
  }
}

which shows that i is upper bounded by a phi instruction generated on the line while (f3_get(n)) n+=2; (which makes a lot more sense 🎉).

Motivated by the above, this PR rewrites our range-analysis tests to not use getAst. This rewrite reveals that a lot of our bounds are phi instructions, and it would probably make sense to improve the string-representation of these bounds somehow. Currently, the toString on the bounds just output whatever toString does on instructions, and on PhiInstruction's that's the word Phi: followed by the first thing in the basic block in which the phi instruction occur (which is why we see =Phi: call to f3_get above). A reasonable follow-up to this PR would be to enrich the output of PhiInstruction's toString to print the name of the variable tracked by the PhiInstruction (if any) so that the test would become:

void* f3_get(int n);
void f3() {
  int n = 0;
  while (f3_get(n)) n+=2;

  for (int i = 0; i < n; i += 2) {
    range(i); // $ range=>=0 SPURIOUS: range="<=Phi[n]: call to f3_get-1" range="<=Phi[n]: call to f3_get-2"
  }
}

I'll leave that to a follow-up PR, though.

[jketema]

I'm not sure this makes things much better, as this makes it very hard to see where abound was actually coming from. I can imagine three solutions: (1) improve the IR Instruction toString predicate, (2) output a location with each instruction, (3) output for each bound both the instruction and the associated getAst. I somewhat prefer 3, as that still gives me something that matches the source text.

I'm getting very confused by all the : in the results now. They're both there to separate instructions and to provide extra context for an instruction. Could we use a different separator for one of these?

[MathiasVP]

I'm getting very confused by all the : in the results now. They're both there to separate instructions and to provide extra context for an instruction. Could we use a different separator for one of these?

Good point. I'll play around with some different separators and see what I like best. ; is an obvious choice since I don't think this will show up in any of the toStrings on the bounds, although it may not be visually distinct enough from :.

[MathiasVP]

@jketema I've changed the separator from : to | now. I much prefer the look of it now, but let me know what you think.

I also have a branch locally that adds immediate strings to PhiInstructions to get the output as shown above, but I'd like to wait with that PR as any change to Instruction.toString probably warrants a DCA run.

[jketema]

I've changed the separator from : to | now. I much prefer the look of it now, but let me know what you think.

Thanks!