[PATCH] D45543: [globalisel] Add a combiner helpers for extending loads and use them in a pre-legalize combiner for AArch64

[Amara Emerson via Phabricator via llvm-commits]

aemerson accepted this revision.
aemerson added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D45543#1074883, @dsanders wrote:

> In https://reviews.llvm.org/D45543#1066099, @aditya_nandakumar wrote:
>
> > Maybe we could have all the tests in one file called prelegalize-combine-extloads.mir
>
>
> I went with three files to match how we've been organizing the tests for the other passes but you raise a good point here. There's a good argument for the combiner being tested with one file per CombinerHelper::try*() function. I think that's probably a better organization.
>
> In https://reviews.llvm.org/D45543#1071617, @aemerson wrote:
>
> > I'm assuming this is in Target/AArch64 because other targets haven't been updated to use the new opcodes yet? We do eventually want to use these representations for every target though right?
>
>
> That's right. As you say, we'll want to support it in every target that supports the extending loads (which is most if not all of the in-tree targets). However, until the new opcodes are legal for those targets, there's not much point in combining them only to revert them back to load+extend in the legalizer.
>
> One other thing to mention is that we don't have a target-independent combiner in GlobalISel at the moment. Each target implements its own combiner(s) and makes use of code in CombinerHelper (where appropriate) to share code. I expect these combines to be used by multiple targets so I've put the bulk of the code in CombinerHelper but each target will need to add a pass and call to it.


Ok makes sense. We still have some work to do on the combiner design front on how to allow targets to select subsets of combines they're interested in so we can have a shared pipeline. I think this is fine for now but it raises the priority for that discussion later.


Repository:
  rL LLVM

https://reviews.llvm.org/D45543