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

Matt Arsenault via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Aug 17 14:32:41 PDT 2020


arsenm added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85965



More information about the llvm-commits mailing list