[PATCH] D99354: [SimpleLoopUnswitch] Port partially invariant unswitch from LoopUnswitch to SimpleLoopUnswitch
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 9 02:08:02 PDT 2021
fhahn added a comment.
Does this patch give the expected speedup on `omnetpp`?
In D99354#2676137 <https://reviews.llvm.org/D99354#2676137>, @jaykang10 wrote:
> @fhahn Can you review this change when you have time please?
please keep the 'common courtesy ping rate' of 1 week in mind (https://llvm.org/docs/CodeReview.html#code-reviews-speed-and-reciprocity), also considering that doing a proper review can take a substantial amount of time for a reviewer and there can be a number of reasons for a delayed response (like holidays, other urgent work).
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:212
+static void buildPartialInvariantUnswitchConditionalBranch(
+ BasicBlock &BB, ArrayRef<Value *> Invariants, bool Direction,
+ BasicBlock &UnswitchedSucc, BasicBlock &NormalSucc, Loop &L,
----------------
`Invariants` here is not really accurate I think. Those are the instructions we need to duplicate outside the loop, right?
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:219
+ Instruction *NewInst = Inst->clone();
+ BB.getInstList().insert(BB.end(), NewInst);
+ RemapInstruction(NewInst, VMap,
----------------
nit: perhaps a bit simpler `NewInst->insertBefore(BB->getTerminator())`
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2015
+ SmallVectorImpl<BasicBlock *> &ExitBlocks,
+ struct IVConditionInfo &PartialIVInfo, DominatorTree &DT, LoopInfo &LI,
+ AssumptionCache &AC,
----------------
`struct` not needed.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2028
"Can only unswitch switches and conditional branch!");
- bool FullUnswitch = SI || BI->getCondition() == Invariants[0];
+ bool PartiallyInvariant = !PartialIVInfo.InstToDuplicate.empty();
+ bool FullUnswitch =
----------------
I think we should probably update `IVConditionInfo` to provide a better way to check if there are partially invariant conditions, e.g. an `isPartiallyInvariant` accessor. WDYT?
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2149
for (auto *SuccBB : UnswitchedSuccBBs) {
- VMaps.emplace_back(new ValueToValueMapTy());
- ClonedPHs[SuccBB] = buildClonedLoopBlocks(
- L, LoopPH, SplitBB, ExitBlocks, ParentBB, SuccBB, RetainedSuccBB,
- DominatingSucc, *VMaps.back(), DTUpdates, AC, DT, LI, MSSAU);
+ // In Partially invariant case, if UnswithcedSuccBB is exit block, do not
+ // clone loop and assigned the UnswitchedSuccBB to ClonedPHs.
----------------
nit: `In the partially invariant case, if UnswitchedSuccBB is an exit block, do not ....`
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2151
+ // clone loop and assigned the UnswitchedSuccBB to ClonedPHs.
+ if (PartiallyInvariant && llvm::any_of(ExitBlocks, [&](BasicBlock *ExitBB) {
+ return ExitBB == SuccBB;
----------------
no need for `llvm::`, also can we just capture `[&SuccBB]`?
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2375
Instruction *UserI = dyn_cast<Instruction>(U.getUser());
- if (!UserI)
+ if (!UserI || PartiallyInvariant)
continue;
----------------
We don't need to execute this loop at all for the `PartiallyInvariant` case, right?
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2692
+ struct IVConditionInfo PartialIVInfo;
for (auto *BB : L.blocks()) {
----------------
`struct` not needed.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2745
+ if (MSSAU &&
+ !llvm::any_of(UnswitchCandidates, [&](auto &TerminatorAndInvariants) {
+ return TerminatorAndInvariants.first == L.getHeader()->getTerminator();
----------------
nit: no `llvm::` should be needed.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2749
+ MemorySSA *MSSA = MSSAU->getMemorySSA();
+ if (auto Info = llvm::hasPartialIVCondition(L, MSSAThreshold, *MSSA, AA)) {
+ LLVM_DEBUG(
----------------
nit: no `llvm::` should be needed
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2878
+ } else if (!PartialIVInfo.InstToDuplicate.empty()) {
+ if (PartialIVInfo.KnownValue->isOneValue() &&
+ SuccBB == BI.getSuccessor(1))
----------------
Do we need to check here that we only do this for blocks where we partially unswitch on their conditions?
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2957
- LLVM_DEBUG(dbgs() << " Unswitching non-trivial (cost = "
- << BestUnswitchCost << ") terminator: " << *BestUnswitchTI
- << "\n");
+ LLVM_DEBUG(dbgs() << " Unswitching non-trivial (cost = " << BestUnswitchCost
+ << ") terminator: " << *BestUnswitchTI << "\n");
----------------
this change seems unrelated?
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3161
+ if (CurrentLoopValid) {
+ if (!PartiallyInvariant)
+ LPM.addLoop(*L);
----------------
can you add a comment here to explain the check?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99354/new/
https://reviews.llvm.org/D99354
More information about the llvm-commits
mailing list