[llvm] r315041 - [CodeExtractor] Fix multiple bugs under certain shape of extracted region
Jakub Kuderski via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 5 20:37:06 PDT 2017
Author: kuhar
Date: Thu Oct 5 20:37:06 2017
New Revision: 315041
URL: http://llvm.org/viewvc/llvm-project?rev=315041&view=rev
Log:
[CodeExtractor] Fix multiple bugs under certain shape of extracted region
Summary:
If the extracted region has multiple exported data flows toward the same BB which is not included in the region, correct resotre instructions and PHI nodes won't be generated inside the exitStub. The solution is simply put the restore instructions right after the definition of output values instead of putting in exitStub.
Unittest for this bug is included.
Author: myhsu
Reviewers: chandlerc, davide, lattner, silvas, davidxl, wmi, kuhar
Subscribers: dberlin, kuhar, mgorny, llvm-commits
Differential Revision: https://reviews.llvm.org/D37902
Added:
llvm/trunk/unittests/Transforms/Utils/CodeExtractor.cpp
Modified:
llvm/trunk/lib/Transforms/Utils/CodeExtractor.cpp
llvm/trunk/unittests/Transforms/Utils/CMakeLists.txt
Modified: llvm/trunk/lib/Transforms/Utils/CodeExtractor.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/CodeExtractor.cpp?rev=315041&r1=315040&r2=315041&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Utils/CodeExtractor.cpp (original)
+++ llvm/trunk/lib/Transforms/Utils/CodeExtractor.cpp Thu Oct 5 20:37:06 2017
@@ -651,19 +651,6 @@ Function *CodeExtractor::constructFuncti
return newFunction;
}
-/// FindPhiPredForUseInBlock - Given a value and a basic block, find a PHI
-/// that uses the value within the basic block, and return the predecessor
-/// block associated with that use, or return 0 if none is found.
-static BasicBlock* FindPhiPredForUseInBlock(Value* Used, BasicBlock* BB) {
- for (Use &U : Used->uses()) {
- PHINode *P = dyn_cast<PHINode>(U.getUser());
- if (P && P->getParent() == BB)
- return P->getIncomingBlock(U);
- }
-
- return nullptr;
-}
-
/// emitCallAndSwitchStatement - This method sets up the caller side by adding
/// the call instruction, splitting any PHI nodes in the header block as
/// necessary.
@@ -736,7 +723,8 @@ emitCallAndSwitchStatement(Function *new
if (!AggregateArgs)
std::advance(OutputArgBegin, inputs.size());
- // Reload the outputs passed in by reference
+ // Reload the outputs passed in by reference.
+ Function::arg_iterator OAI = OutputArgBegin;
for (unsigned i = 0, e = outputs.size(); i != e; ++i) {
Value *Output = nullptr;
if (AggregateArgs) {
@@ -759,6 +747,34 @@ emitCallAndSwitchStatement(Function *new
if (!Blocks.count(inst->getParent()))
inst->replaceUsesOfWith(outputs[i], load);
}
+
+ // Store to argument right after the definition of output value.
+ auto *OutI = dyn_cast<Instruction>(outputs[i]);
+ if (!OutI)
+ continue;
+ // Find proper insertion point.
+ Instruction *InsertPt = OutI->getNextNode();
+ // Let's assume that there is no other guy interleave non-PHI in PHIs.
+ if (isa<PHINode>(InsertPt))
+ InsertPt = InsertPt->getParent()->getFirstNonPHI();
+
+ assert(OAI != newFunction->arg_end() &&
+ "Number of output arguments should match "
+ "the amount of defined values");
+ if (AggregateArgs) {
+ Value *Idx[2];
+ Idx[0] = Constant::getNullValue(Type::getInt32Ty(Context));
+ Idx[1] = ConstantInt::get(Type::getInt32Ty(Context), FirstOut + i);
+ GetElementPtrInst *GEP = GetElementPtrInst::Create(
+ StructArgTy, &*OAI, Idx, "gep_" + outputs[i]->getName(), InsertPt);
+ new StoreInst(outputs[i], GEP, InsertPt);
+ // Since there should be only one struct argument aggregating
+ // all the output values, we shouldn't increment OAI, which always
+ // points to the struct argument, in this case.
+ } else {
+ new StoreInst(outputs[i], &*OAI, InsertPt);
+ ++OAI;
+ }
}
// Now we can emit a switch statement using the call as a value.
@@ -801,75 +817,13 @@ emitCallAndSwitchStatement(Function *new
break;
}
- ReturnInst *NTRet = ReturnInst::Create(Context, brVal, NewTarget);
+ ReturnInst::Create(Context, brVal, NewTarget);
// Update the switch instruction.
TheSwitch->addCase(ConstantInt::get(Type::getInt16Ty(Context),
SuccNum),
OldTarget);
- // Restore values just before we exit
- Function::arg_iterator OAI = OutputArgBegin;
- for (unsigned out = 0, e = outputs.size(); out != e; ++out) {
- // For an invoke, the normal destination is the only one that is
- // dominated by the result of the invocation
- BasicBlock *DefBlock = cast<Instruction>(outputs[out])->getParent();
-
- bool DominatesDef = true;
-
- BasicBlock *NormalDest = nullptr;
- if (auto *Invoke = dyn_cast<InvokeInst>(outputs[out]))
- NormalDest = Invoke->getNormalDest();
-
- if (NormalDest) {
- DefBlock = NormalDest;
-
- // Make sure we are looking at the original successor block, not
- // at a newly inserted exit block, which won't be in the dominator
- // info.
- for (const auto &I : ExitBlockMap)
- if (DefBlock == I.second) {
- DefBlock = I.first;
- break;
- }
-
- // In the extract block case, if the block we are extracting ends
- // with an invoke instruction, make sure that we don't emit a
- // store of the invoke value for the unwind block.
- if (!DT && DefBlock != OldTarget)
- DominatesDef = false;
- }
-
- if (DT) {
- DominatesDef = DT->dominates(DefBlock, OldTarget);
-
- // If the output value is used by a phi in the target block,
- // then we need to test for dominance of the phi's predecessor
- // instead. Unfortunately, this a little complicated since we
- // have already rewritten uses of the value to uses of the reload.
- BasicBlock* pred = FindPhiPredForUseInBlock(Reloads[out],
- OldTarget);
- if (pred && DT && DT->dominates(DefBlock, pred))
- DominatesDef = true;
- }
-
- if (DominatesDef) {
- if (AggregateArgs) {
- Value *Idx[2];
- Idx[0] = Constant::getNullValue(Type::getInt32Ty(Context));
- Idx[1] = ConstantInt::get(Type::getInt32Ty(Context),
- FirstOut+out);
- GetElementPtrInst *GEP = GetElementPtrInst::Create(
- StructArgTy, &*OAI, Idx, "gep_" + outputs[out]->getName(),
- NTRet);
- new StoreInst(outputs[out], GEP, NTRet);
- } else {
- new StoreInst(outputs[out], &*OAI, NTRet);
- }
- }
- // Advance output iterator even if we don't emit a store
- if (!AggregateArgs) ++OAI;
- }
}
// rewrite the original branch instruction with this new target
Modified: llvm/trunk/unittests/Transforms/Utils/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Transforms/Utils/CMakeLists.txt?rev=315041&r1=315040&r2=315041&view=diff
==============================================================================
--- llvm/trunk/unittests/Transforms/Utils/CMakeLists.txt (original)
+++ llvm/trunk/unittests/Transforms/Utils/CMakeLists.txt Thu Oct 5 20:37:06 2017
@@ -9,6 +9,7 @@ set(LLVM_LINK_COMPONENTS
add_llvm_unittest(UtilsTests
ASanStackFrameLayoutTest.cpp
Cloning.cpp
+ CodeExtractor.cpp
FunctionComparator.cpp
IntegerDivision.cpp
Local.cpp
Added: llvm/trunk/unittests/Transforms/Utils/CodeExtractor.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Transforms/Utils/CodeExtractor.cpp?rev=315041&view=auto
==============================================================================
--- llvm/trunk/unittests/Transforms/Utils/CodeExtractor.cpp (added)
+++ llvm/trunk/unittests/Transforms/Utils/CodeExtractor.cpp Thu Oct 5 20:37:06 2017
@@ -0,0 +1,69 @@
+//===- CodeExtractor.cpp - Unit tests for CodeExtractor -------------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Transforms/Utils/CodeExtractor.h"
+#include "llvm/AsmParser/Parser.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/Dominators.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/IR/Verifier.h"
+#include "llvm/IRReader/IRReader.h"
+#include "llvm/Support/SourceMgr.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+TEST(CodeExtractor, ExitStub) {
+ LLVMContext Ctx;
+ SMDiagnostic Err;
+ std::unique_ptr<Module> M(parseAssemblyString(R"invalid(
+ define i32 @foo(i32 %x, i32 %y, i32 %z) {
+ header:
+ %0 = icmp ugt i32 %x, %y
+ br i1 %0, label %body1, label %body2
+
+ body1:
+ %1 = add i32 %z, 2
+ br label %notExtracted
+
+ body2:
+ %2 = mul i32 %z, 7
+ br label %notExtracted
+
+ notExtracted:
+ %3 = phi i32 [ %1, %body1 ], [ %2, %body2 ]
+ %4 = add i32 %3, %x
+ ret i32 %4
+ }
+ )invalid",
+ Err, Ctx));
+
+ Function *Func = M->getFunction("foo");
+ SmallVector<BasicBlock *, 3> Candidates;
+ for (auto &BB : *Func) {
+ if (BB.getName() == "body1")
+ Candidates.push_back(&BB);
+ if (BB.getName() == "body2")
+ Candidates.push_back(&BB);
+ }
+ // CodeExtractor requires the first basic block
+ // to dominate all the other ones.
+ Candidates.insert(Candidates.begin(), &Func->getEntryBlock());
+
+ DominatorTree DT(*Func);
+ CodeExtractor CE(Candidates, &DT);
+ EXPECT_TRUE(CE.isEligible());
+
+ Function *Outlined = CE.extractCodeRegion();
+ EXPECT_TRUE(Outlined);
+ EXPECT_FALSE(verifyFunction(*Outlined));
+}
+} // end anonymous namespace
More information about the llvm-commits
mailing list