[PATCH] D27853: [CodeGenPrep]Restructure promoting Ext to form ExtLoad

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml marked an inline comment as done.
junbuml added a comment.

Thanks Quentin for the review and congrats for your new baby. 
Added comments in the code and see my inline comments. Please let me know any other question or comment.

Thanks,
Jun



================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4329
+          !(StressExtLdPromotion || NewCreatedInstsCost <= ExtCost ||
+           (ExtOperand->hasOneUse() || hasSameExtUse(ExtOperand, *TLI))))
+        continue;
----------------
qcolombet wrote:
> junbuml wrote:
> > qcolombet wrote:
> > > I am confused we are checking that we can form an extended load and that is profitable.
> > > The condition shouldn't use the ! operator, right?
> > In this function (tryToPromoteExts()), we speculatively promote extensions and save extensions profitably moved up, in ProfitablyMovedExts.
> > 
> > Note that ProfitablyMovedExts contains profitable extensions which is not necessarily fed by a load. To find an extended load, we use canFormExtLd(), which will determine if the final output of ProfitablyMovedExts contains an extension which can be used to form an extendable load.
> > 
> > The final output of ProfitablyMovedExts after recursion will contain the last profitable extensions moved up during the speculative promotion, and it will be used in my second patch (https://reviews.llvm.org/D28680) which moves aarch64-type-promotion pass to CGP   
> If we record all the extensions that are profitable shouldn't we do:
> 
> ```
> if (**!**isa<LoadInst>(ExtOperand) **||** !(StressExtLdPromotion || NewCreatedInstsCost <= ExtCost ||
>            (ExtOperand->hasOneUse() || hasSameExtUse(ExtOperand, *TLI)))))
> ```
> 
> I.e., we should invert the first half of the original condition as well.

Since we record all the extensions that are profitable, we should not "continue" for  non-load instructions added in  ProfitablyMovedExts line 4324 in it's recursive call. This check should be done for load which is added in line 4275 as it could potentially be merged into an extld.  In this change, ProfitablyMovedExts is only used to find an extendable load in canFormExtLd(). Non-load instructions could be promoted in D28680 which migrate aarch64-type-promotion to CGP based on this change.


https://reviews.llvm.org/D27853