This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Generalize `InstructionSelector` Match Tables
ClosedPublic

Authored by Pierre-vh on Jun 26 2023, 4:52 AM.

Details

Summary

Makes InstructionSelector.h/InstructionSelectorImpl.h generic so the match tables can also be used for the combiner.

Some notes:

  • Coverage was made an optional parameter of executeMatchTable, combines won't use it for now.
  • GIPFP_ -> GICXXPred_ so it's more generic. Those are just C++ predicates and aren't PatFrag-specific.
  • Pass the MatcherState directly to testMIPredicate_MI, the combiner will need it.

Diff Detail

Event Timeline

Pierre-vh created this revision.Jun 26 2023, 4:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2023, 4:52 AM
Pierre-vh requested review of this revision.Jun 26 2023, 4:52 AM
Herald added a project: Restricted Project. · View Herald Transcript
lkail added a subscriber: lkail.Jun 26 2023, 4:57 AM
arsenm added inline comments.Jun 27 2023, 10:43 AM
llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
77–80

What's the point of having 4 copies of 0?

448–451

wrong word why

llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h
328–329

could this be a const reference? I guess this is a preexisting issue

Pierre-vh marked 3 inline comments as done.

Address comments

Pierre-vh added inline comments.Jun 28 2023, 12:45 AM
llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
77–80

(It was here before, I didn't touch it, just moved it) No point really. I'd be fine with just having a GICXXPred_Invalid

Though, if that's ok, I'd rather fix it in a later diff such as D153757 (which adds more flags here). It makes rebasing less annoying, and none of those patch land unless they all land anyway.

llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h
328–329

It was indeed already there but I fixed it anyway, and also fixed the APInt one.

arsenm accepted this revision.Jun 28 2023, 5:18 AM

This is just renaming things anyway

This revision is now accepted and ready to land.Jun 28 2023, 5:18 AM
foad added inline comments.Jul 12 2023, 3:58 AM
llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
456

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?

Pierre-vh added inline comments.Jul 12 2023, 4:02 AM
llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
456

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.

foad added inline comments.Jul 12 2023, 4:15 AM
llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
456

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?

Pierre-vh added inline comments.Jul 12 2023, 4:41 AM
llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
456

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

Pierre-vh marked an inline comment as done.Jul 12 2023, 6:22 AM
Pierre-vh added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
456