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

Aditya Nandakumar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Sep 10 22:51:08 PDT 2020


aditya_nandakumar added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp:38
+      return false; // Conservatively return false.
+  }
+
----------------
arsenm wrote:
> aditya_nandakumar wrote:
> > 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.
> It may be a canonicalization, but I also don't think it's the MIRBuilder's job to ensure everything is canonical 
I'm not sure if you mean MIRBuilder here or the CSE mechanism. I was talking about the mechanism (the fact that we do it through builders is just implementation detail). My personal take on this is that ending up with a mechanism that doesn't guarantee CSE (assuming instructions can be CSEd) even if you enabled it fully by default, is not ideal.
Perhaps to address compile time for this issue (say O0 pipeline), we could try to observe which opcodes are responsible and selectively disable these with the CSEConfig. When I did measurements a while back among all opcodes CSEd, G_CONSTANT + FCONSTANT were responsible for about 75-80% of the hits in the test suites I looked at (out of tree), and these are usually right at the beginning and hence not a big problem for dominance queries taking too long. Obviously the longer term solution of O(1) dominance queries could be considered, but right now I don't have a better + cleaner suggestion.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87297



More information about the llvm-commits mailing list