Page MenuHomePhabricator

[SROA] rewritePartition()/findCommonType(): if uses have conflicting type, try getTypePartition() before falling back to largest integral use type (PR47592)
ClosedPublic

Authored by lebedev.ri on Oct 3 2020, 12:58 PM.

Diff Detail

Event Timeline

lebedev.ri created this revision.Oct 3 2020, 12:58 PM
lebedev.ri requested review of this revision.Oct 3 2020, 12:58 PM
lebedev.ri updated this revision to Diff 296053.Oct 4 2020, 9:19 AM

Fix debuginfo test

I guess if we're relying on the allocated type of the alloca anyway, preferring it over an integer type isn't terrible.

Really, though, we should avoid relying on the allocated type where possible. Here, we could check if any of the load/store operations use a pointer type, and choose a pointer type in that case.

I guess if we're relying on the allocated type of the alloca anyway, preferring it over an integer type isn't terrible.

Really, though, we should avoid relying on the allocated type where possible. Here, we could check if any of the load/store operations use a pointer type, and choose a pointer type in that case.

FWIW, both of these make lots of sense to me.

nlopes added a subscriber: nlopes.Oct 6 2020, 3:17 AM

I guess if we're relying on the allocated type of the alloca anyway, preferring it over an integer type isn't terrible.

Really, though, we should avoid relying on the allocated type where possible. Here, we could check if any of the load/store operations use a pointer type, and choose a pointer type in that case.

Agreed. But until LLVM removes pointer sub-types it's convenient to get the alloca type right to avoid bitcast on every access anyway.
When pointer sub-types goes away, I guess all this code in SROA to find the right type for alloca would go away, but as you say it would have to be replaced with code to get the right load/store type instead. (FWIW Alive2's alloca only takes the number of bytes to allocate as argument)
So I see this patch as a step in the right direction.

I guess if we're relying on the allocated type of the alloca anyway, preferring it over an integer type isn't terrible.

Really, though, we should avoid relying on the allocated type where possible. Here, we could check if any of the load/store operations use a pointer type, and choose a pointer type in that case.

Agreed. But until LLVM removes pointer sub-types it's convenient to get the alloca type right to avoid bitcast on every access anyway.
When pointer sub-types goes away, I guess all this code in SROA to find the right type for alloca would go away, but as you say it would have to be replaced with code to get the right load/store type instead. (FWIW Alive2's alloca only takes the number of bytes to allocate as argument)
So I see this patch as a step in the right direction.

So i don't do this blindly to find out that is'a bad idea, can we agree on the baseline here?
How should this be done properly? Instead of relying on the allocation type, would the D88842's approach be applicable here?

I've looked now that rGe00f189d392d is done, and i see two-ish main origin sources of introduction inttoptr/ptrtoint that weren't there before:

  1. instcombine's SimplifyAnyMemTransfer() - given memcpy of a pointer-sized thingy, we lower it to a load+store of an integer
  2. instcombine's combineLoadToOperationType() - CastInst::isNoopCast() needs to be updated that these casts are not no-op. (isEliminableCastPair() too, i guess)
nlopes added a comment.Oct 6 2020, 3:52 AM

I guess if we're relying on the allocated type of the alloca anyway, preferring it over an integer type isn't terrible.

Really, though, we should avoid relying on the allocated type where possible. Here, we could check if any of the load/store operations use a pointer type, and choose a pointer type in that case.

Agreed. But until LLVM removes pointer sub-types it's convenient to get the alloca type right to avoid bitcast on every access anyway.
When pointer sub-types goes away, I guess all this code in SROA to find the right type for alloca would go away, but as you say it would have to be replaced with code to get the right load/store type instead. (FWIW Alive2's alloca only takes the number of bytes to allocate as argument)
So I see this patch as a step in the right direction.

So i don't do this blindly to find out that is'a bad idea, can we agree on the baseline here?
How should this be done properly? Instead of relying on the allocation type, would the D88842's approach be applicable here?

I don't know SROA's code in detail to answer. I guess the point is that we don't want to introduce type-punning loads. So if the original program did a pointer load, the optimized program should do the same pointer load. Even if that means we can't combine 2 loads from the same address because one is pointer and another int. (see below the byte type proposal, as it allows you to combine loads into byte loads and then do a cast).

I've looked now that rGe00f189d392d is done, and i see two-ish main origin sources of introduction inttoptr/ptrtoint that weren't there before:

  1. instcombine's SimplifyAnyMemTransfer() - given memcpy of a pointer-sized thingy, we lower it to a load+store of an integer

This one requires a new byte type. We need it to support C's char anyway. Byte type doesn't have to be 1 byte, so maybe let's do it like integers: b1, b2, b3, etc.
Value with byte type could hold a value (or part of one) of any other LLVM type. No arithmetic operations are allowed on these types, only load/store and non-type punning type casts.

@nlopes thank you very much!

So since everyone seems to agree that this seems fine as a temporary step in the right direction,
does anyone feel like stamping it?

lebedev.ri updated this revision to Diff 296413.Oct 6 2020, 4:27 AM
lebedev.ri edited the summary of this revision. (Show Details)

Rebased, NFC.

I've measured, and @ -O3 as of vanilla llvm test-suite + RawSpeed,
this results in +0.05% more bitcasts, -5.51% less inttoptr and -1.05% less ptrtoint
(at the end of middle-end opt pipeline)

efriedma accepted this revision.Oct 6 2020, 6:13 PM

LGTM

This revision is now accepted and ready to land.Oct 6 2020, 6:13 PM

LGTM

Thank you for the review.

This revision was landed with ongoing or failed builds.Oct 6 2020, 11:20 PM
This revision was automatically updated to reflect the committed changes.