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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp:38
+      return false; // Conservatively return false.
+  }
+
----------------
aditya_nandakumar wrote:
> aemerson wrote:
> > aditya_nandakumar wrote:
> > > This change looks concerning.
> > > If this method returns false, it implies that A does not dominate B, and `getDominatingInstrForID` ends up moving the instruction to the current position. Is this what you're trying to do? (I suspect not).
> > Right, I'd forgotten that `getDominatingInstrForID()` assumes that A and B are in the same block before its calls dominates(). I'll try passing in a return parameter to signal back when the search was aborted early?
> That would work. Though I'm not sure if this is the best way to go about - what we'll end up with is a MachineFunction that may or may not have CSE'd instructions (even though it could be). If CSE is being relied upon for canonicalization, this may not be the right approach (where depending on where an instruction is, it may or may not be CSEd).
> Alternatively we can make this a part of the CSEConfig which allows targets to opt out of this behavior?
I don't think CSE should be a guaranteed property. I don't think we need a knob for this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87297