reactorlabs / rir

GNU General Public License v2.0
62 stars 18 forks source link

Backwards analysis required for ForceDominance #1263

Open skrynski opened 8 months ago

skrynski commented 8 months ago

At: https://github.com/reactorlabs/rir/blob/00402235a12976a48e88f397f0bac0e76f2fa035/rir/src/compiler/opt/force_dominance.cpp#L112

To determine whether a promise is unused anywhere else and therefore can be lowered, a forward analysis does not suffice. See file attached. If a promise is not used 'so far' within a particular branch, it does not mean it is not used in any other branches and, therefore, is not necessarily a candidate. FDGVN.txt

Notice that, in the file, the Deopt BB comes from a dead CheckPoint. Maybe we should schedule a Checkpoint cleanup at the very end of ForceDominance.

Also, we should revisit the method isUnused.

This is the only issue I did not solve when working on #1250. The bug is exposed when the budget is lifted from the pass scheduler. Sometimes ForceDominance and GVN will fight each other while doing and undoing changes, getting stuck in a loop. For example, ForceDominance would push MkArg instructions down to exit BBs, and GNU would push them back up. To expose the bug, run : PIR_WARMUP=2 PIR_DEOPT_CHAOS=50000 bin/R -f ../../external/custom-r/tests/Examples/utils-Ex.R