[PATCH] D85965: [GlobalISel] Add a combine for ashr(shl x, c), c --> sext_inreg x, c'

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

In D85965#2222602 <https://reviews.llvm.org/D85965#2222602>, @arsenm wrote:

> In D85965#2218403 <https://reviews.llvm.org/D85965#2218403>, @aemerson wrote:
>
>> In D85965#2218363 <https://reviews.llvm.org/D85965#2218363>, @arsenm wrote:
>>
>>> In D85965#2218352 <https://reviews.llvm.org/D85965#2218352>, @aemerson wrote:
>>>
>>>> In D85965#2218104 <https://reviews.llvm.org/D85965#2218104>, @arsenm wrote:
>>>>
>>>>> Seems like it's missing a legality check, although I'm unclear on what the overall strategy for those is supposed to be
>>>>
>>>> This is intended to run prelegalizer so I don't think we need it.
>>>
>>> Combines should probably try to support both. There's fragments of half implemented support for this
>>
>> Every combine should check legality for each instruction it wants to generate? That seems like way overkill to me. The majority of the combines we do will be pre-legalize with post being used to clean up legalizer related things or do some lowering for isel. I think it should be done on as needed basis by whichever target needs a specific combine to be done post-legalize. Unless we have a better mechanism to do this that doesn't involve manually calling into LI every time,
>
> Currently we have CombinerHelper::isLegalOrBeforeLegalizer, which some combines use. CombinerInfo has IllegalOpsAllowed and LegalizeIllegalOps (which has the comment  // TODO: Make use of this.) I guess the intent was these could re-legalize as it goes, and some reasonable subset of legality checks could be use for a profitability heuristic?
>
> I think the majority of interesting combines are post-legalize, particularly when it comes to bit operations. Before legalization the MIR isn't particularly different than the source optimized IR, and the legalizer introduces new patterns.

I think those flags were intended for the full-fledged tablegen pattern combiner to use, but your point still stands. I'll add a legality check here but it seems like we need some more infrastructure to make use of IllegalOpsAllowed on the patterns in a more concise way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85965