This is an archive of the discontinued LLVM Phabricator instance.

[CodeGenPrep] move aarch64-type-promotion to CGP
ClosedPublic

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

Details

Summary

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.
include/llvm/Analysis/TargetTransformInfo.h
503

Maybe \return instead of \brief Return

504

A const & would probably be more appropriate.

lib/CodeGen/CodeGenPrepare.cpp
943

CI is not valid anymore at this point.

2595–2596

Add a comment for the new arg

4278

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).

4366

Add a comment explaining what this method does.

4501

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

4504

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?

4542

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

4546

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.

4575

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

lib/Target/AArch64/AArch64TargetTransformInfo.cpp
631

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.

lib/CodeGen/CodeGenPrepare.cpp
943

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

4501

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

4504

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

4542

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

4546

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

4575

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
lib/CodeGen/CodeGenPrepare.cpp
4578

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.

lib/CodeGen/CodeGenPrepare.cpp
4548

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.