[InstCombine] When should InstCombine indicate something was changed to the pass managers?

InstCombine consists of iteration of the following two steps

  1. Building the worklist over the function. Including dead code elimination, constant folding, and constant argument folding.
  2. Processing the worklist.

InstCombine currently only reports a change to the pass manager if more than 1 iteration is executed. And we only do additional iterations when worklist processing changed something.

But the first iteration could do deadcode elmination or constant folding during the worklist build, but make no changes during the processing. Do those count as changes that should be passed up to the pass manager?

There’s an additional odd quirk here, if we constant fold an argument during the worklist build we do return a changed flag from the worklist build which gets ORed with the change flag for the worklist processing. This can force another iteration even if the worklist processing itself doesn’t make any change. I’ve seen this happen during some InstCombine runs where we constant fold a GEP ConstantExpr just to change an i32 to i64 for one of the indices.Clearly we shouldn’t be forcing a subsequent iteration just because the worklist build changed something but the worklist itself didn’t.

Thanks,

InstCombine consists of iteration of the following two steps

1. Building the worklist over the function. Including dead code elimination,
constant folding, and constant argument folding.
2. Processing the worklist.

InstCombine currently only reports a change to the pass manager if more than
1 iteration is executed. And we only do additional iterations when worklist
processing changed something.

But the first iteration could do deadcode elmination or constant folding
during the worklist build, but make no changes during the processing. Do
those count as changes that should be passed up to the pass manager?

In the current world, the semantic associated to the return value of a
pass execution is, IMHO: "did this pass modify the unit of IR on which
is working on?"
Folding + removing instructions have visible effects, so, yes, I think
it should return `true`.
The current logic has the unfortunate effect of invalidating all the
analyses. When the new pass manager will be in place, you may be a
little bit more granular as the return value of a pass is a Preserved
set of analyses rather than a boolean value.

There's an additional odd quirk here, if we constant fold an argument during
the worklist build we do return a changed flag from the worklist build which
gets ORed with the change flag for the worklist processing. This can force
another iteration even if the worklist processing itself doesn't make any
change. I've seen this happen during some InstCombine runs where we constant
fold a GEP ConstantExpr just to change an i32 to i64 for one of the
indices.Clearly we shouldn't be forcing a subsequent iteration just because
the worklist build changed something but the worklist itself didn't.

Maybe this can be improved, although I think one iteration more hardly
matters for compile time, YMMV.