cdl-saarland / rv

RV: A Unified Region Vectorizer for LLVM
Other
102 stars 15 forks source link

Fix core dump in CloneLoop #30

Closed mppf closed 5 years ago

mppf commented 5 years ago

In trying to integrate RV with the Chapel compiler I ran into a core dump inside RV.

If I apply the following patch, I get an assertion " can only clone single exit loops" instead.

diff --git a/src/transform/loopCloner.cpp b/src/transform/loopCloner.cpp
index 27e191c..b2b3fa6 100644
--- a/src/transform/loopCloner.cpp
+++ b/src/transform/loopCloner.cpp
@@ -28,11 +28,11 @@ struct LoopCloner {
   LoopCloneInfo
   CloneLoop(Loop & L, ValueToValueMapTy & valueMap) {
     auto * loopPreHead = L.getLoopPreheader();
-    auto * preTerm = loopPreHead->getTerminator();
     auto & loopHead = *L.getHeader();
     auto * loopExiting = L.getExitingBlock();
     assert(loopPreHead && loopExiting && " can only clone single exit loops");

+    auto * preTerm = loopPreHead->getTerminator();
     auto * splitBranch = BranchInst::Create(&loopHead, &loopHead, ConstantInt::getTrue(loopHead.getContext()), loopPreHead);

     // clone all basic blocks

The root cause for the Chapel integration issue was that the frontend needed to call rv::addPreparatoryPasses in addition to rv::addOuterLoopVectorizer. I've fixed that now, but it would be nice if there were a clearer error here (say, one that mentioned the need to call addPreparatoryPasses). Additionally it would be nice if https://github.com/cdl-saarland/rv/wiki/How-to-use-RV%27s-outer-loop-vectorizer-in-your-LLVM-frontend included the need for rv::addPreparatoryPasses (and probably rv::addWholeFunctionVectorizer, rv::addOuterLoopVectorizer, rv::addCleanupPasses(PM); ).

It would also work just fine for me if RV provided registerRVPasses (or something like it) as an exported function. I'm just as happy to call addPreparatoryPasses etc.

simoll commented 5 years ago

These are all valid points. I've made some changes in that direction now:

simoll commented 5 years ago

Can we close this?

mppf commented 5 years ago

Sure