LICM does not detect defs from virtual method calls

[LouisJenkinsCS]

Summary of Problem

Loop Invariant Code Motion (LICM) seems to cause some sort of 'undefined behavior' in that it causes some things to be compiled away entirely. Like how the tuple variable seems to be given undefined/uninitialized values. Furthermore, how the entire loop gets optimized away if we do not add that extra 'writeln' (of which is not needed with the --no-loop-invariant-code-motion flag).

As well, this is another opportunity to bring another issue that @e-kayrakli had made in my place, #6542 , which can also be reproduced here by calling fn() without capturing it in the the tuples (a,b) will produce a segfault (with and without --no-loop-invariant-code-motion flag).

The amount of time I've spent double, triple, and quadruple checking my data structure thinking that I corrupted my data structure or something was exhausting.

Steps to Reproduce

Source Code:

TIO (Online Compiler)

With --no-loop-invariant-code-motion

TIO (Online Compiler)

class D {
  proc fn() : (bool, int) {
    halt("Should not be called...");
  }
}

class C : D {
  proc fn() : (bool, int) {
    return (false, 1);
  }
}

var c : D = new C();

var (a, b) = (true, 0);
for 1 .. 5 {
  // This shows that the value, if printed prior to usage, seems to cause an issue
  if b != 0 then writeln("a: ", a, ", b: ", b);
  (a, b) = c.fn();
}
// Note: If this line is removed, the loop itself will not run (optimized away?)
writeln("Needed so it will not be compiled away on TIO...");

Output:

a: true, b: 180388626485
a: true, b: 180388626485
a: true, b: 180388626485
a: true, b: 180388626485
Needed so it will not be compiled away on TIO...

Output (With '--no-loop-invariant-code-motion'):

a: false, b: 1
a: false, b: 1
a: false, b: 1
a: false, b: 1
Needed so it will not be compiled away on TIO...

[LouisJenkinsCS]

@ben-albrecht

Here is the issue and MWE you wanted me to make.

@ronawho

Ben said you'd be interested in this.

[LouisJenkinsCS]

@e-kayrakli

I think I came across this bug before as well, it was when I reported that 'Dynamic Dispatch was causing undefined behavior' for my FIFO queue, and you said that dynamic dispatch was just exposing some race condition. Code used the exact same pattern here (code is snapshot of almost 2 months ago). I spent days trying to figure out what was wrong, at least now I know its a bug.

[ben-albrecht]

For context on why this considered a "gating issue":

fn() is a simplified remove() from the Collections Module.

[ronawho]

I think I have a good lead on the issue here. I'm going to be away for the next 2 weeks, but I think I can get a fix in for the release.

@ben-albrecht could add a .future test for this bug and link to it in the issue description?

[ronawho]

@ben-albrecht and/or @LouisJenkinsCS #7315 should resolve this. Can you confirm that this resolves the issue you were running into and if so remove the bug-note in Collection.chpl

[LouisJenkinsCS]

I'll make it a priority to get around to testing it myself (rerunning all tests without --no-loop-invariant-code-motion) and removing any trace of those bug notes tonight. I'll update here when I do.

[LouisJenkinsCS]

All tests succeed, I'll make a new PR soon.

[ronawho]

Cool! Thanks for checking

[LouisJenkinsCS]

@ronawho

I spoke too soon. I went back and made a few minor changes to the unit tests (refactored as originally it was written to 'step around' the whole LICM bug), and now I get this:

A while loop with a constant condition

I'll make a PR soon, and @ mention you on it.

[LouisJenkinsCS]

Posting this here:

#7322 (comment)

Its link to my comment in case not notified