[llvm] [InstCombine] Remove some of the complexity-based canonicalization (PR #91185)
via llvm-commits
llvm-commits at lists.llvm.org
Tue May 7 18:13:38 PDT 2024
goldsteinn wrote:
> > > > > > Can you run it? Ill try and get -stats to work locally and post them here.
> > > > >
> > > > >
> > > > > I cannot run it as LLVM doesn't support the cost estimation of struct types :(
> > > >
> > > >
> > > > Err misunderstanding, I mean can you just generate your normal diffs. Ill do the cost estimation stuff locally and just post the results here.
> > >
> > >
> > > Done. The IR diff basically looks fine to me.
> > > The main problem is that some `icmp pred A, B` and `icmp swap(pred) B, A` pairs are not CSEed now. See [dtcxzyw/llvm-opt-benchmark#583 (comment)](https://github.com/dtcxzyw/llvm-opt-benchmark/pull/583#discussion_r1591786430).
> >
> >
> > Ah, truthfully that and commutative binops imo make a case for keeping this as is.
>
> Our main CSE passes (EarlyCSE and GVN) have support for handling commutative operations. I think in this case the failure is probably from a pass like SimplifyCFG where we may fail to hoist/sink "non-identical" instructions.
>
> The problem with relying on InstCombine complexity-based canonicalization for this purpose is that it's very weak. It will handle the case where the icmp has an instruction and argument operand, but if both are instructions, then they will not be canonicalized and we're back to the same problem. So I think to handle this properly we still need explicit commutative operation support.
What about making the matchers for commutative ops default to the commutative version (thinks like cmp could be included).
We do that in the new `sd_match` API.
Or have the builder do the canonicalization... although that seems tricky.
https://github.com/llvm/llvm-project/pull/91185
More information about the llvm-commits
mailing list