Page MenuHomePhabricator

[InstCombine] Adjusting bswap pattern matching to hold for And/Shift mixed case
ClosedPublic

Authored by opaparo on Apr 17 2018, 11:44 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

opaparo created this revision.Apr 17 2018, 11:44 AM

Thanks for coming back to this. I think this patch is fine as one more match of a potential IR variant, but...
Since the time we started the discussion in D41353, there was a proposal to improve matching for ctpop and ctlz in D45173. I'm curious what others think about the fact that we match bswap/bitreverse in regular instcombine. Would it be better to match all of those intrinsics in the same place? Do we distinguish these based on how often we expect them to occur?

This revision was not accepted when it landed; it landed in state Needs Review.Apr 19 2018, 10:29 AM
This revision was automatically updated to reflect the committed changes.
kparzysz reopened this revision.Apr 19 2018, 10:34 AM

Sorry, I pasted a wrong revision in a commit message.

kparzysz commandeered this revision.Apr 19 2018, 10:41 AM
kparzysz edited reviewers, added: opaparo; removed: kparzysz.

I screwed it up completely. Let me try to fix it.

kparzysz updated this revision to Diff 143127.Apr 19 2018, 10:49 AM

Hopefully the right diff was restored after the commit mismatch fiasco.

@opaparo Please commandeer this revision, I only took over to restore the diff that got accidentally overwritten.

opaparo set the repository for this revision to rL LLVM.Apr 19 2018, 11:40 AM
opaparo commandeered this revision.Apr 19 2018, 11:46 AM
opaparo edited reviewers, added: kparzysz; removed: opaparo.

Restoring the ownership of this revision.

Thanks for coming back to this. I think this patch is fine as one more match of a potential IR variant, but...
Since the time we started the discussion in D41353, there was a proposal to improve matching for ctpop and ctlz in D45173. I'm curious what others think about the fact that we match bswap/bitreverse in regular instcombine. Would it be better to match all of those intrinsics in the same place? Do we distinguish these based on how often we expect them to occur?

Ping.
Is there a decision regarding this matter?

spatel accepted this revision.Apr 27 2018, 9:09 AM

Thanks for coming back to this. I think this patch is fine as one more match of a potential IR variant, but...
Since the time we started the discussion in D41353, there was a proposal to improve matching for ctpop and ctlz in D45173. I'm curious what others think about the fact that we match bswap/bitreverse in regular instcombine. Would it be better to match all of those intrinsics in the same place? Do we distinguish these based on how often we expect them to occur?

Ping.
Is there a decision regarding this matter?

I haven't seen anything. For reference, I mentioned ctpop/ctlz in the context of D45986 too, so I think we should do something to organize these optimizations, but I'm not sure what's best.
That said, I think this patch is fine as an enhancement of the existing code, so LGTM.

This revision is now accepted and ready to land.Apr 27 2018, 9:09 AM
This revision was automatically updated to reflect the committed changes.

I haven't seen anything. For reference, I mentioned ctpop/ctlz in the context of D45986 too, so I think we should do something to organize these optimizations, but I'm not sure what's best.
That said, I think this patch is fine as an enhancement of the existing code, so LGTM.

Sorry, I was away last week. My understanding is that the distinction between the regular and the aggressive instcombine is based on how expensive the individual transformations are. If we had an infinite computational power, everything would be included in the regular instcombine. Based on that, I think that this patch is ok as is.