[PATCH] D74165: [x86] [DAGCombine] Prefer shifts of constant widths.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:3420-3424
+  virtual bool
+  shiftOrRotateIsFasterWithConstantShiftAmount(const SDNode *N,
+                                               CombineLevel Level) const {
+    return false;
+  }
----------------
arsenm wrote:
> jlebar wrote:
> > arsenm wrote:
> > > jlebar wrote:
> > > > arsenm wrote:
> > > > > I think we generally have too many, overly specific queries like this. Is there any real reason to NOT do this on any target?
> > > > I am also not thrilled about adding another query like this that pretends to be general but really means "should I do this particular peephole optimization?"
> > > > 
> > > > I gated it conditionally on the architecture because ISTM that this transformation could be a pessimation on platforms where shift-by-register is as fast as shift-by-immediate.  We're replacing 1 x shift-by-register + 1 x select with 2 x shift-by-immediate + 1 x select.  AFAICT e.g. on Coretex-A75 this would not be an improvement.
> > > GlobalISel avoids this problem by having every combine be an explicit opt in if I can encourage you to also do this there
> > Happy to consider it, but I'm not sure what the suggestion is exactly.  (Or at least, this seems like we're making the combine be an explicit opt-in, so I don't see how it's different from your suggestion.)  Can you point me to the prior art as used in non-globalisel?
> I mean SelectionDAG is bad and am just generally disappointed there isn't more work going on for globalisel optimizations, and everyone seems to still just be working on SelectionDAG
>From personal expirience - i only look at seldag because that is what is on the current (codegen) path.
Doing stuff for GISel would currently result in no benefit to codegen on X86, which kinda misses the point.
Is there an actual plan for X86 target to migrate to GISel?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74165