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

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp:38
+      return false; // Conservatively return false.
+  }
+
----------------
arsenm wrote:
> 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
It's certainly a nice property to have. For eg, we can check if a new instruction we're building already exists and is being used just by trying building a new instruction and checking for uses of the def, which could be useful for combines (and we have at least one combine that does this in our backend). This could perhaps be enabled by other APIs but is currently unavailable.
I also think of CSE as a canonicalization and later passes can rematerialize as necessary.
Also @aemerson , does this trigger in those workloads you care about (and hence if this change is necessary at all) since you mentioned that you arrived at the limit by going one order of magnitude higher.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87297