Move removal of exception edges to before collecting fixable predecessors

[a7ehuo]

Blocks could have been removed when the exception edges
of an empty goto block are removed. The removal should
take place before the IN edges are saved in fixablePreds.

Fixes: eclipse-openj9/openj9#15305

Signed-off-by: Annabelle Huo Annabelle.Huo@ibm.com

[a7ehuo]

@jdmpapin May I ask you to review this change? Thanks!

[jdmpapin]

Come to think of it, while block will remain reachable and will still have predecessors, we have no guarantee that there will be any remaining fixablePreds. It might be more straightforward to just remove the exception successors before collecting the fixablePreds, e.g. right after determining the value of containsTreesOtherThanGoto:

if (!containsTreesOtherThanGoto && !block->getExceptionSuccessors().empty())
   {
   // Structure repair doesn't deal well with exception successors in this case.
   if (!performTransformation("...", ...))
      continue; // the exception successors remain, so skip this block

   TR::CFGEdgeList &excSuccs = block->getExceptionSuccessors();
   while (!excSuccs.empty())
      cfg->removeEdge(excSuccs.front());
   }

That way, fixablePreds would never get out of date

[a7ehuo]

e.g. right after determining the value of containsTreesOtherThanGoto

If the removal of exception edges is put right after determining the value of containsTreesOtherThanGoto, the exception edges could be removed from a block that doesn't proceed to be removed later on, because of the code like below could skip this block after the removal:

omr/compiler/optimizer/LocalOpts.cpp

Lines 3433 to 3442 in e287e61

	if (block->getPredecessors().empty())
	continue;
	if (block->isLoopInvariantBlock())
	continue;
	TR::Block *destBlock = block->getSuccessors().front()->getTo()->asBlock();
	if (destBlock == block)
	continue; // No point trying to "update" predecessors

@jdmpapin Would it be an issue to remove the exception edges if the block contains only goto and the block is not removed? just double checking, although I don't think it's gonna be an issue.

If there is no issue, I plan to move the removal of exception edges right before updating fixablePreds so that if the block is not removed for any reason before updating fixablePreds, we don't need to remove its exception edge.

[a7ehuo]

@jdmpapin Updated the implementation to move the removal of exception edges right before updating fixablePreds in 4cf26fa. Ready for another review. Thanks!

[jdmpapin]

@jdmpapin Would it be an issue to remove the exception edges if the block contains only goto and the block is not removed? just double checking, although I don't think it's gonna be an issue.

No, there's no issue. A block consisting only of goto (and possibly asynccheck) will not throw, so all of its outgoing exception edges are dead, and those edges can be removed regardless of anything else

[jdmpapin]

Jenkins build all

[a7ehuo]

Updated the PR title and description to reflect the final change that moves the removal of exception edges right before updating fixablePreds

[jdmpapin]

The zos build hit an infrastructure failure:

ERROR: Error cloning remote repo 'origin'

Jenkins build zos

[jdmpapin]

The PPC Linux failure is #6556. That bug is supposed to be intermittent, so I'm restarting the job in the hope that it will pass and allow the other tests to run

Jenkins build plinux

[jdmpapin]

Jenkins build plinux

[jdmpapin]

Jenkins build plinux

[jdmpapin]

The failure is still just #6556. It keeps happening here, but it's unrelated (known prior) to this PR. Merging

[jdmpapin]

Forgot to approve before merging and can't add my approval now. Please take this comment as my approval ✔️