[PATCH v2 1/1] utils: Fix segfault in flattencfg
Jan Vesely
jan.vesely at rutgers.edu
Mon Aug 11 13:03:42 PDT 2014
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.
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>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140811/d66d0fa8/attachment.sig>
More information about the llvm-commits
mailing list