[PATCH v2 1/1] utils: Fix segfault in flattencfg
Matt Arsenault
Matthew.Arsenault at amd.com
Mon Aug 11 12:03:01 PDT 2014
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140811/f5ad6ac6/attachment.html>
More information about the llvm-commits
mailing list