[PATCH] D87297: [GlobalISel] Add bailout thresholds to CSEMIRBuilder::dominates() and the localizer.

[Amara Emerson via Phabricator via llvm-commits]

aemerson marked an inline comment as not done.
aemerson added a comment.

In D87297#2264152 <https://reviews.llvm.org/D87297#2264152>, @qcolombet wrote:

> Hi Amara,
>
>> dominates(A, B) tries to determine dominance by walking an iterator from the beginning of the block potentially until the end. In extremely large BBs this can result in very long compile times, since this is called often from the artifact combiner.
>
> Instead of disabling the check, I think it would be best to fix the underlying compile time issue. The solution here would be to switch the list of MachineInstr to numbered instructions so that dominance checks become O(1).
> I had meant to look at this for quite some time but didn't get a chance to do it.
> Something similar has been done on LLVM IR  (see https://reviews.llvm.org/D51664). I wish it was done in a way that could have benefited the Machine IR like I suggested in https://reviews.llvm.org/D51664#1389884, but this ship has sailed!
>
> I understand this is a big under taking so I am fine with the threshold approach in the meantime.
>
> I leave the final approval to Aditya and Matt.
>
> Cheers,
> -Quentin

Yes I saw that thread, it's a shame we didn't have a shared solution for MBB too. There's probably more places where we have hidden super-linear compile time waiting to be triggered.

It's actually not quite as simple as I previously thought. The current logic in `getDominatingInstrForID` seems to be this:

  MachineInstr *A = CSE_lookup_in_same_bb(B)
  if (!dominates(A, B))
      move A before B such that dominates(A, B) is true
  else
      return pointer to A

If we hit our threshold, what should `getDominatingInstrForID()` do? Perhaps clone the instruction A instead?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87297/new/

https://reviews.llvm.org/D87297