Page MenuHomePhabricator

[GlobalISel] Combine and (lshr x, cst), mask -> ubfx x, cst, width

Authored by paquette on Mar 24 2021, 10:56 AM.



Only do this post-legalization for now

This is generic now.

Diff Detail

Event Timeline

paquette created this revision.Mar 24 2021, 10:56 AM
paquette requested review of this revision.Mar 24 2021, 10:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2021, 10:56 AM
Herald added a subscriber: wdng. · View Herald Transcript
arsenm added inline comments.Mar 24 2021, 11:08 AM

The amounts could be a different type from the value, so I guess I'll need to fix that in the instruction definition



paquette updated this revision to Diff 345197.May 13 2021, 10:05 AM
paquette added a reviewer: jroelofs.

Rebase + move to AArch64-only to match the G_SBFX combine. Maybe this can be cannibalized into something more generic.

jroelofs added inline comments.May 19 2021, 6:55 PM
289 ↗(On Diff #345197)

What is the range of AndImm at this point? I'm worried that we'll have UB if the addition overflows.

paquette updated this revision to Diff 346771.May 20 2021, 9:57 AM
  • Fix UB by doing all the computations on unsigned ints (which is what SDAG does anyway)
  • Simplify the logic a little bit more
  • Add boundary testcases which use signed + unsigned max integer values

(Maybe there should be a m_UICst(...) matcher or something...?)

arsenm added inline comments.May 20 2021, 1:41 PM
282–285 ↗(On Diff #346771)

This is the only target specific thing here which can be replaced with a legality check on UBFX

308 ↗(On Diff #346771)



Why does this need asserts?

paquette added inline comments.May 20 2021, 2:01 PM

The only-enable-rule stuff only works with asserts.

paquette added inline comments.Tue, May 25, 11:34 AM
282–285 ↗(On Diff #346771)

The annoying part (for AArch64) is that the legality checks don't work with custom legalization.

I'm not sure what the best way to handle custom actions is.

arsenm added inline comments.Thu, May 27, 5:55 PM
282–285 ↗(On Diff #346771)

That's what we usually end up adding TLI hooks for, although they're terrible

paquette updated this revision to Diff 348599.Fri, May 28, 4:47 PM
paquette edited the summary of this revision. (Show Details)

Make this a generic combine + add a target hook to define when it should be enabled.

jroelofs accepted this revision.Sat, May 29, 3:56 PM



Depending on subtarget-specific latencies, this may be worthwhile even when the shift has more than one use if lat(ubfx) < lat(and) + lat(lshr).

This revision is now accepted and ready to land.Sat, May 29, 3:56 PM
Matt added a subscriber: Matt.Mon, May 31, 9:47 AM