[PATCH] D153755: [GlobalISel] Generalize `InstructionSelector` Match Tables

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh marked an inline comment as done.
Pierre-vh added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h:456
   CodeGenCoverage *CoverageInfo = nullptr;
   GISelKnownBits *KnownBits = nullptr;
   MachineFunction *MF = nullptr;
----------------
Pierre-vh wrote:
> foad wrote:
> > Pierre-vh wrote:
> > > foad wrote:
> > > > Hi @Pierre-vh, after merging this patch into a downstream branch I find I have to write this in some of our C++ combines:
> > > > ```
> > > >   struct KnownBits Op0KnownBits = KnownBits->getKnownBits(Reg0);
> > > > ```
> > > > This is a bit ugly because of the name clash between the KnownBits struct and this KnownBits field. I guess this is not a new problem, since you did not change this line, but it is now affecting more code.
> > > > 
> > > > Do you think it would make sense to change the name of this field?
> > > Sure, I wouldn't mind changing the name. Should we just name it `KB`?
> > > Do you want me to make a patch?
> > > 
> > > Those fields really ought to be cleaned-up some day anyway. Once the older backend is removed I plan to do a pass over this code and eliminate as much redundancy as possible. Things aren't a good state right now because we still need to allow downstream users to use the older combiner backend. e.g. the whole "CombinerInfo" class is useless with the new combiner.
> > Yes `KB` sounds good to me, thanks!
> > 
> > If CombinerInfo is useless with the new scheme then can we get rid of AMDGPU derived classes like AMDGPUPostLegalizerCombinerInfo right now?
> We could but it wouldn't give us anything I think. IIRC there's also some things that expect a CombinerInfo derived class.
> I want to get rid of CombinerInfo directly and make the Combiner pass structure similar to ISel. It's going to be a breaking change for downstream users of the old backend so I'm waiting until it's been deprecated for a few weeks
D155082


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153755