Page MenuHomePhabricator

[CodeGenPrep] move aarch64-type-promotion to CGP

Authored by junbuml on Jan 13 2017, 7:56 AM.



Move the aarch64-type-promotion pass within the existing type promotion framework in CGP.
This change also support forking sexts when a new sext is required for promotion.
Note that change is based on D27853 and I am submitting this out early to provide a better idea on D27853.

Diff Detail


Event Timeline

junbuml updated this revision to Diff 84307.Jan 13 2017, 7:56 AM
junbuml retitled this revision from to [CodeGenPrep] move aarch64-type-promotion to CGP.
junbuml updated this object.
junbuml added a subscriber: llvm-commits.
junbuml updated this revision to Diff 84369.Jan 13 2017, 1:59 PM
junbuml updated this revision to Diff 84693.Jan 17 2017, 10:01 AM
junbuml updated this revision to Diff 92181.Mar 17 2017, 12:23 PM

Rebased it after committing r298114.

Kindly ping. Please let me know any comment.

qcolombet requested changes to this revision.Mar 28 2017, 10:16 AM
qcolombet added inline comments.
526 ↗(On Diff #92181)

Maybe \return instead of \brief Return

527 ↗(On Diff #92181)

A const & would probably be more appropriate.

1096 ↗(On Diff #92181)

CI is not valid anymore at this point.

2824 ↗(On Diff #92181)

Add a comment for the new arg

4526 ↗(On Diff #92181)

Why is the example removed here?
Edit: ok I see you moved it.
Maybe have the comment from the other function reference that function (or the other way around).

4621 ↗(On Diff #92181)

Add a comment explaining what this method does.

4757 ↗(On Diff #92181)

The part mentioning 64 bits is, I believe, not in sync with the code.
This depends on what shouldConsiderAddressTypePromotion returns.

4760 ↗(On Diff #92181)

I wonder how much of this logic applies to other target. What I am saying is that this may be beneficial for AArch64, but is it true for X86 for instance?

4798 ↗(On Diff #92181)

What purpose does this serve?
I mean the next rollback will be a noop, right?

4802 ↗(On Diff #92181)

This should only be in the else block, right?

Indeed, if canFormExtLd returns true, the changes are committed and there isn't anything else added to the transaction AFAICT. Thus this call does nothing.

4808 ↗(On Diff #92181)

For this condition, same question, how does this apply to something that is not AArch64?

633 ↗(On Diff #92181)

Add comments

This revision now requires changes to proceed.Mar 28 2017, 10:16 AM
junbuml updated this revision to Diff 93517.Mar 30 2017, 12:46 PM
junbuml edited edge metadata.
junbuml marked 11 inline comments as done.

Addressed Quentin's comments. Please take a look and let me know any comments.

1096 ↗(On Diff #92181)

No need to add CI in RemovedExtInsts because none of extension removed here is considered in optimizeExt().

4757 ↗(On Diff #92181)

Yes, I move this comment in AArch64TTIImpl::shouldConsiderAddressTypePromotion().

4760 ↗(On Diff #92181)

I believe this should target specific. So I move this into AArch64TTIImpl by adding another parameter (AllowPromotionWithoutCommonHeader) decided in shouldConsiderAddressTypePromotion().

4798 ↗(On Diff #92181)

Yes, look like this is useless. I removed it. If canFormExtLd is true, return from this function (optimizeExt) instead.

4802 ↗(On Diff #92181)

In case case canFormExtLd returns false, we should check ATPConsiderable and if ATPConsiderable is false rollback should happen.

4808 ↗(On Diff #92181)

This should be target specific. So this should be mentioned in AArch64TTIImpl .

junbuml updated this revision to Diff 93530.Mar 30 2017, 1:01 PM
qcolombet added inline comments.Mar 30 2017, 5:34 PM
4800 ↗(On Diff #93530)

Could you move that into its own helper function?

junbuml updated this revision to Diff 93691.Mar 31 2017, 12:27 PM
junbuml marked an inline comment as done.

Addressed Quentin's comment by adding a new method, performAddressTypePromotion().

qcolombet accepted this revision.Apr 1 2017, 10:42 AM

LGTM with a nitpick.

Also, I am guessing you'll have a follow-up patch for removing the AArch64AddressTypePromotion pass.

4803 ↗(On Diff #93691)

No else after return statement

This revision is now accepted and ready to land.Apr 1 2017, 10:42 AM
This revision was automatically updated to reflect the committed changes.

I will post a follow-up patch to remove the AArch64AddressTypePromotion pass shortly.