[PATCH] D143787: [X86] Add new pass `X86FixupISel` for fixing up machine-instruction selection.

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n marked 2 inline comments as done.
goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Target/X86/X86FixupISel.cpp:12
+// attempting to get the "right" instruction can break patterns. This pass
+// is not meant to do any analysis. It is only meant to be make transformations
+// that are guaranteed to be doable.
----------------
RKSimon wrote:
> goldstein.w.n wrote:
> > RKSimon wrote:
> > > What do you mean by analysis? I'm hoping we can use scheduler models in the future to drive some transforms here.
> > I mean something like transforming `vpermq ymm` <-> `vshufd ymm` should not be put in this file, as its not always valid (requires analyzing the mask to see if no lane crosses and repeated / mask is only in pairs of 2). OTOH `vpermilps <-> vshufd` are always interchangable. I'll make the logic more clear.
> Agreed, anything involving valuetracking etc. shouldn't be done this late.
> 
> However, I don't see any problem with replacing instructions based on immediates that are part of the instruction - we already do this for load folding, commutation, domain switching etc.
> Agreed, anything involving valuetracking etc. shouldn't be done this late.
> 
> However, I don't see any problem with replacing instructions based on immediates that are part of the instruction - we already do this for load folding, commutation, domain switching etc.

Its not inherently wrong, but my feeling is that belongs in X86ISelLowering/X86ISelDAGToDAG/td. I think it would work for a few instructions, but it seems like the kind of thing that will explode the complexity of the file over time (even the shuffle logic with in X86ISelLowering is extremely complex/convoluted with the clean APIs the DAG has). I think that kind of complexity on a file operation on machine instr will be bug-prone. Generally, we *could* do it here, but I don't think we *should*.


================
Comment at: llvm/lib/Target/X86/X86FixupISel.cpp:1
+//===-- X86FixupISels.cpp - use or replace ISel instructions -----------===//
+//
----------------
RKSimon wrote:
> pengfei wrote:
> > craig.topper wrote:
> > > Referring to ISel and running the pass so far from instruction selection is perhaps misleading.
> > I have the same concern, if it is required for `vpermli*`, maybe just name it `FixupVpermli`.
> The hope is to add other instruction combos in the future - given we're trying to fixup for a mixture of tuning/scheduler - maybe X86FixupTuning.cpp ?
Renamed to `X86FixupInstTuning.cpp` (though `FixupTuning` was a bit generic about what exactly is being tuned).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143787