Details
Diff Detail
Event Timeline
include/llvm/Analysis/TargetTransformInfo.h | ||
---|---|---|
529 | Maybe \return instead of \brief Return | |
530 | A const & would probably be more appropriate. | |
lib/CodeGen/CodeGenPrepare.cpp | ||
1105 | CI is not valid anymore at this point. | |
2831–2832 | Add a comment for the new arg | |
4534 | Why is the example removed here? | |
4629 | Add a comment explaining what this method does. | |
4765 | The part mentioning 64 bits is, I believe, not in sync with the code. | |
4768 | 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? | |
4770–4771 | 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. | |
4792 | What purpose does this serve? | |
4797 | For this condition, same question, how does this apply to something that is not AArch64? | |
lib/Target/AArch64/AArch64TargetTransformInfo.cpp | ||
629 | Add comments |
Addressed Quentin's comments. Please take a look and let me know any comments.
lib/CodeGen/CodeGenPrepare.cpp | ||
---|---|---|
1105 | No need to add CI in RemovedExtInsts because none of extension removed here is considered in optimizeExt(). | |
4765 | Yes, I move this comment in AArch64TTIImpl::shouldConsiderAddressTypePromotion(). | |
4768 | I believe this should target specific. So I move this into AArch64TTIImpl by adding another parameter (AllowPromotionWithoutCommonHeader) decided in shouldConsiderAddressTypePromotion(). | |
4770–4771 | In case case canFormExtLd returns false, we should check ATPConsiderable and if ATPConsiderable is false rollback should happen. | |
4792 | Yes, look like this is useless. I removed it. If canFormExtLd is true, return from this function (optimizeExt) instead. | |
4797 | This should be target specific. So this should be mentioned in AArch64TTIImpl . |
lib/CodeGen/CodeGenPrepare.cpp | ||
---|---|---|
4800 | Could you move that into its own helper function? |
LGTM with a nitpick.
Also, I am guessing you'll have a follow-up patch for removing the AArch64AddressTypePromotion pass.
lib/CodeGen/CodeGenPrepare.cpp | ||
---|---|---|
4798 | No else after return statement |
I will post a follow-up patch to remove the AArch64AddressTypePromotion pass shortly.
Maybe \return instead of \brief Return