Correctly report modified status for LoopDistribute

[serge-sans-paille]

Summary

Related to https://reviews.llvm.org/D80916

[foad]

llvm/lib/Transforms/Scalar/LoopDistribute.cpp

789

It doesn't really matter but... aren't these variables usually called "Changed"?

791–792

Would it be possible to delay this splitting until we know that the checks below won't fail? Or is that difficult to do?

[serge-sans-paille]

Move code that unconditionally modifiy the loop *after* the structural check, to avoid returning false while some changes happened.

It passes validation, and I did a quick check, the change looks fine, but I'd be happy to take an extra review from someone more familiar with the codebase.

[serge-sans-paille]

This comment was removed by serge-sans-paille.

[serge-sans-paille]

Ooops™ uploaded the wrong patch, restoring the correct version.

[foad]

llvm/lib/Transforms/Scalar/LoopDistribute.cpp

792–802

Adding @arsenm who last touched this bit of code.

[serge-sans-paille]

up?

[foad]

I have absolutely no idea whether it is safe to move that code, sorry.

[jdoerfert]

LGTM.

llvm/lib/Transforms/Scalar/LoopDistribute.cpp

809

Nit: unsure why you added braces but I think we prefer without.

llvm/lib/Transforms/Scalar/LoopDistribute.cpp

if (Pred.getComplexity() > (IsForced.getValueOr(false)
       return fail("TooManySCEVRuntimeChecks",
                   "too many SCEV run-time checks needed.\n");
 
     if (!IsForced.getValueOr(false) && hasDisableAllTransformsHint(L))
       return fail("HeuristicDisabled", "distribution heuristic disabled");
 
     LLVM_DEBUG(dbgs() << "\nDistributing loop: " << *L << "\n");
     // We're done forming the partitions set up the reverse mapping from
     // instructions to partitions.
     Partitions.setupPartitionIdOnInstructions();
 
-    // To keep things simple have an empty preheader before we version or clone
-    // the loop.  (Also split if this has no predecessor, i.e. entry, because we
-    // rely on PH having a predecessor.)
-    if (!PH->getSinglePredecessor() || &*PH->begin() != PH->getTerminator())
-      SplitBlock(PH, PH->getTerminator(), DT, LI);
-
     // If we need run-time checks, version the loop now.
     auto PtrToPartition = Partitions.computePartitionSetForPointers(*LAI);
     const auto *RtPtrChecking = LAI->getRuntimePointerChecking();
     const auto &AllChecks = RtPtrChecking->getChecks();
     auto Checks = includeOnlyCrossPartitionChecks(AllChecks, PtrToPartition,
                                                   RtPtrChecking);
 
     if (LAI->hasConvergentOp() && !Checks.empty()) {
       return fail("RuntimeCheckWithConvergent",
                   "may not insert runtime check with convergent operation");
     }
 
+    // To keep things simple have an empty preheader before we version or clone
+    // the loop.  (Also split if this has no predecessor, i.e. entry, because we
+    // rely on PH having a predecessor.)
+    if (!PH->getSinglePredecessor() || &*PH->begin() != PH->getTerminator())
+      SplitBlock(PH, PH->getTerminator(), DT, LI);
+
     if (!Pred.isAlwaysTrue() || !Checks.empty()) {
       assert(!LAI->hasConvergentOp() && "inserting illegal loop versioning");
 
       MDNode *OrigLoopID = L->getLoopID();
 
       LLVM_DEBUG(dbgs() << "\nPointers:\n");
       LLVM_DEBUG(LAI->getRuntimePointerChecking()->printChecks(dbgs(), Checks));
       LoopVersioning LVer(*LAI, L, LI, DT, SE, false);