[PATCH] D33847: [PartialInlining] Enhance code outliner to sink locals declared outside the outline region

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:166
+
+bool CodeExtractor::isLegalToShrinkwrapLifetimeMarkers(
+    Instruction *Addr) const {
----------------
Why we choose to skip BitCast, GetElementPtr, ..., Alloca, but treat other instructions like BinaryOp instruction conservatively. Why not just check memory access instructions like load/store/call and treat them conservatively, but skip all the other instructions?


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:187-197
+      case Instruction::Load: {
+        LoadInst *LI = dyn_cast<LoadInst>(&II);
+        Value *Mem = LI->getOperand(0);
+        // Global variable not can not be aliased with locals.
+        if (dyn_cast<Constant>(Mem))
+          break;
+        Value *Base = Mem->stripInBoundsConstantOffsets();
----------------
Can store have similar logic as load here?


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:292-300
+        for (User *U : Addr->users()) {
           IntrinsicInst *IntrInst = dyn_cast<IntrinsicInst>(U);
           if (IntrInst) {
             if (IntrInst->getIntrinsicID() == Intrinsic::lifetime_start)
               LifeStart = IntrInst;
             if (IntrInst->getIntrinsicID() == Intrinsic::lifetime_end)
               LifeEnd = IntrInst;
----------------
I think it is possible one alloca has multiple pairs of lifetime_start and lifetime_end. Like loop unrolling may create such case. It is also possible for one lifetime_start to have multiple lifetime_ends. But I guess the most common case in inlined blocks is one pair of lifetime_start and lifetime_end, so we may just add some check to avoid the other uncommon cases.     


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:337-348
       for (User *U : AI->users()) {
-        if (U->stripPointerCasts() == AI) {
+
+        if (U->stripInBoundsConstantOffsets() == AI) {
+          SinkLifeStart = false;
+          HoistLifeEnd = false;
           Instruction *Bitcast = cast<Instruction>(U);
+          Markers = GetLifeTimeMarkers(Bitcast, SinkLifeStart, HoistLifeEnd);
----------------
If AI have multiple bitcasts, we will only sink one of them and may cause problem?


https://reviews.llvm.org/D33847