[PATCH v2 1/1] utils: Fix segfault in flattencfg
Tom Stellard
tom at stellard.net
Mon Aug 11 14:57:07 PDT 2014
On Mon, Aug 11, 2014 at 04:03:42PM -0400, Jan Vesely wrote:
> On Mon, 2014-08-11 at 12:03 -0700, Matt Arsenault wrote:
> > On 08/10/2014 02:47 PM, Jan Vesely wrote:
> > > On Sat, 2014-08-09 at 16:26 -0400, Tom Stellard wrote:
> > >> >On 08/09/2014 03:52 PM, Jan Vesely wrote:
> > >>> > >On Fri, 2014-08-08 at 14:18 -0700, Matt Arsenault wrote:
> > >>>> > >>On Aug 8, 2014, at 1:41 PM, Jan Vesely<jan.vesely at rutgers.edu> wrote:
> > >>>> > >>
> > >>>>> > >>>v2: continue iterating through the rest of the bb
> > >>>>> > >>> use for loop
> > >>>>> > >>>
> > >>>>> > >>>Signed-off-by: Jan Vesely<jan.vesely at rutgers.edu>
> > >>>>> > >>>---
> > >>>>> > >>>lib/Transforms/Utils/FlattenCFG.cpp | 9 +++++----
> > >>>>> > >>>1 file changed, 5 insertions(+), 4 deletions(-)
> > >>>>> > >>>
> > >>>>> > >>>diff --git a/lib/Transforms/Utils/FlattenCFG.cpp b/lib/Transforms/Utils/FlattenCFG.cpp
> > >>>>> > >>>index 51ead40..4eb3e3d 100644
> > >>>>> > >>>--- a/lib/Transforms/Utils/FlattenCFG.cpp
> > >>>>> > >>>+++ b/lib/Transforms/Utils/FlattenCFG.cpp
> > >>>>> > >>>@@ -238,9 +238,13 @@ bool FlattenCFGOpt::FlattenParallelAndOr(BasicBlock *BB, IRBuilder<> &Builder,
> > >>>>> > >>> // Do branch inversion.
> > >>>>> > >>> BasicBlock *CurrBlock = LastCondBlock;
> > >>>>> > >>> bool EverChanged = false;
> > >>>>> > >>>- while (1) {
> > >>>>> > >>>+ for (;CurrBlock != FirstCondBlock;
> > >>>>> > >>>+ CurrBlock = CurrBlock->getSinglePredecessor()) {
> > >>>>> > >>> BranchInst *BI = dyn_cast<BranchInst>(CurrBlock->getTerminator());
> > >>>>> > >>> CmpInst *CI = dyn_cast<CmpInst>(BI->getCondition());
> > >>>>> > >>>+ if (!CI)
> > >>>>> > >>>+ continue;
> > >>>>> > >>>+
> > >>>>> > >>> CmpInst::Predicate Predicate = CI->getPredicate();
> > >>>>> > >>> // Canonicalize icmp_ne -> icmp_eq, fcmp_one -> fcmp_oeq
> > >>>>> > >>> if ((Predicate == CmpInst::ICMP_NE) || (Predicate == CmpInst::FCMP_ONE)) {
> > >>>>> > >>>@@ -248,9 +252,6 @@ bool FlattenCFGOpt::FlattenParallelAndOr(BasicBlock *BB, IRBuilder<> &Builder,
> > >>>>> > >>> BI->swapSuccessors();
> > >>>>> > >>> EverChanged = true;
> > >>>>> > >>> }
> > >>>>> > >>>- if (CurrBlock == FirstCondBlock)
> > >>>>> > >>>- break;
> > >>>>> > >>>- CurrBlock = CurrBlock->getSinglePredecessor();
> > >>>>> > >>> }
> > >>>>> > >>> return EverChanged;
> > >>>>> > >>> }
> > >>>> > >>
> > >>>> > >>Needs tests
> > >>> > >
> > >>> > >sorry, I misunderstood what you meant by test the first time.
> > >>> > >
> > >>> > >I wanted to add a flattencfg pass specific test but ran into problems
> > >>> > >
> > >>> > >is there an easy way to run only the flattencfg pass?
> > >>> > >opt -pass_name works only for analytical passes,
> > >>> > >and llc ... -stop-after=flattencfg reports pass not registered.
> > >>> > >
> > >> >
> > >> >You should be able to run any pass using opt. Did you look at the
> > >> >output of opt -help-hidden ?
> > >> >
> > >> >Actually, flattencfg doens't show up for me in the help options.
> > >> >This may be because it doesn't get initialized with the other passes in
> > >> >lib/Transforms/Scalar/Scalar.cpp. Try adding the initialize call there
> > >> >and see if that makes it work with opt.
> > > this worked, thank you. v3 is attached.
> > >
> > > the test is a simplified situation from the mad_sat implementation.
> > > the crash occurs at the end of b0.
> > >
> > > jan
> > >
> > >> >
> > >> >-Tom
> > >> >
> > >> >
> > >>> > >I guess I'll have to add a codegen test.
> > >>> > >
> > >>> > >I also found the parallel{or,and}ifcollapse tests, that are marked XFAIL
> > >>> > >for unrelated reason. Should I try to fix those or add a new test?
> > >>> > >
> > >>> > >jan
> > >>> > >
> > >> >
> > > -- Jan Vesely <jan.vesely at rutgers.edu>
> > >
> > > 0001-utils-Fix-segfault-in-flattencfg.patch
> > >
> > >
> > > From 509d65e62475b7d34966c38b0177648482a31ce8 Mon Sep 17 00:00:00 2001
> > > From: Jan Vesely<jan.vesely at rutgers.edu>
> > > Date: Sat, 9 Aug 2014 16:41:29 -0400
> > > Subject: [PATCH v3 1/1] utils: Fix segfault in flattencfg
> > >
> > > v2: continue iterating through the rest of the bb
> > > use for loop
> > >
> > > v3: initialize FlattenCFG pass in ScalarOps
> > > add test
> > >
> > > Signed-off-by: Jan Vesely<jan.vesely at rutgers.edu>
> > > ---
> > > lib/Transforms/Scalar/Scalar.cpp | 1 +
> > > lib/Transforms/Utils/FlattenCFG.cpp | 9 +++++----
> > > test/Transforms/Util/flattencfg.ll | 23 +++++++++++++++++++++++
> > > 3 files changed, 29 insertions(+), 4 deletions(-)
> > > create mode 100644 test/Transforms/Util/flattencfg.ll
> > >
> > > diff --git a/lib/Transforms/Scalar/Scalar.cpp b/lib/Transforms/Scalar/Scalar.cpp
> > > index 12df676..4bebb42 100644
> > > --- a/lib/Transforms/Scalar/Scalar.cpp
> > > +++ b/lib/Transforms/Scalar/Scalar.cpp
> > > @@ -38,6 +38,7 @@ void llvm::initializeScalarOpts(PassRegistry &Registry) {
> > > initializeDSEPass(Registry);
> > > initializeGVNPass(Registry);
> > > initializeEarlyCSEPass(Registry);
> > > + initializeFlattenCFGPassPass(Registry);
> > > initializeIndVarSimplifyPass(Registry);
> > > initializeJumpThreadingPass(Registry);
> > > initializeLICMPass(Registry);
> > > diff --git a/lib/Transforms/Utils/FlattenCFG.cpp b/lib/Transforms/Utils/FlattenCFG.cpp
> > > index 51ead40..4eb3e3d 100644
> > > --- a/lib/Transforms/Utils/FlattenCFG.cpp
> > > +++ b/lib/Transforms/Utils/FlattenCFG.cpp
> > > @@ -238,9 +238,13 @@ bool FlattenCFGOpt::FlattenParallelAndOr(BasicBlock *BB, IRBuilder<> &Builder,
> > > // Do branch inversion.
> > > BasicBlock *CurrBlock = LastCondBlock;
> > > bool EverChanged = false;
> > > - while (1) {
> > > + for (;CurrBlock != FirstCondBlock;
> > > + CurrBlock = CurrBlock->getSinglePredecessor()) {
> > > BranchInst *BI = dyn_cast<BranchInst>(CurrBlock->getTerminator());
> > > CmpInst *CI = dyn_cast<CmpInst>(BI->getCondition());
> > > + if (!CI)
> > > + continue;
> > > +
> > > CmpInst::Predicate Predicate = CI->getPredicate();
> > > // Canonicalize icmp_ne -> icmp_eq, fcmp_one -> fcmp_oeq
> > > if ((Predicate == CmpInst::ICMP_NE) || (Predicate == CmpInst::FCMP_ONE)) {
> > > @@ -248,9 +252,6 @@ bool FlattenCFGOpt::FlattenParallelAndOr(BasicBlock *BB, IRBuilder<> &Builder,
> > > BI->swapSuccessors();
> > > EverChanged = true;
> > > }
> > > - if (CurrBlock == FirstCondBlock)
> > > - break;
> > > - CurrBlock = CurrBlock->getSinglePredecessor();
> > > }
> > > return EverChanged;
> > > }
> > > diff --git a/test/Transforms/Util/flattencfg.ll b/test/Transforms/Util/flattencfg.ll
> > > new file mode 100644
> > > index 0000000..949d7a2
> > > --- /dev/null
> > > +++ b/test/Transforms/Util/flattencfg.ll
> > > @@ -0,0 +1,23 @@
> > > +; RUN: opt -flattencfg -S < %s | FileCheck %s
> > > +
> > > +; Function Attrs: nounwind
> > > +; CHECK-LABEL: test_1
> > > +define void @test_1(i32 %in_a) #0 {
> > > +entry:
> > > + %cmp0 = icmp eq i32 %in_a, -1
> > > + %cmp1 = icmp ne i32 %in_a, 0
> > > + %cond0 = and i1 %cmp0, %cmp1
> > > + br i1 %cond0, label %b0, label %b1
> > > +
> > > +b0: ; preds = %entry
> > > + %cmp2 = icmp eq i32 %in_a, 0
> > > + %cmp3 = icmp ne i32 %in_a, 1
> > > + %cond1 = or i1 %cmp2, %cmp3
> > > + br i1 %cond1, label %exit, label %b1
> > > +
> > > +b1: ; preds = %entry, %b0
> > > + br label %exit
> > > +
> > > +exit: ; preds = %entry, %b0, %b1
> > > + ret void
> > > +}
> > > -- 1.9.3
> >
> > LGTM, although the test isn't actually checking anything. The test
> > should either check something, or not use filecheck if this is supposed
> > to be a no-op on this. CHECK-LABELs should also include the @ on the
> > function name.
>
> The test is just a simplified version of the situation that causes a
> crash without the patch. It does not really care if the code gets
> transformed at all.
>
If the code is getting transformed, it will be useful to check for
the correct transformation, since there aren't many tests for this
pass. If nothing is transformed then you can leave it as is and just
add a comment explaining that this test is just making sure the pass
doesn't crash. You should still use FileCheck, though, even if all you
have is a CHECK-LABEL.
-Tom
> I assumed the file might be later extended with actual transformation
> tests. I can drop the Filecheck part if you prefer.
>
> I'll post v4 before pushing
>
> jan
>
> --
> Jan Vesely <jan.vesely at rutgers.edu>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list