[llvm] r257253 - [BranchFolding] Set correct mem refs
Michael Zolotukhin via llvm-commits
llvm-commits at lists.llvm.org
Sun Jan 10 19:42:13 PST 2016
> On Jan 10, 2016, at 6:53 PM, 박준모 <junmoz.park at samsung.com <mailto:junmoz.park at samsung.com>> wrote:
>
> Hi Benjamin, Michael.
Hi Junmo,
>
> I couldn't expect that array's memory reference information can be changed with -O3 option.
> Why I used combination of "Opt -O3 & llc" is that I want to represent real world's problem.(Branch folding can be occurred after Function Inlining.)
I see. LLVM exploits a modular structure, so (almost) every transformation can be tested independently on others. That doesn’t mean all transformations work only on artificial tests - all of them actually target some real world problems, but testing them in isolation is just much more stable.
> But this makes test failure on some machine. (Index of arrayidx1 can be changed, when optimization pass is changed.)
>
> So I change my test code likes below. I hope below code is ok for everyone. If there are no objections, I will recommit this.
I haven’t looked at the code, but if you didn’t change it, I suppose it’s still considered “ok to commit”. However, I have some comments regarding the test - please see below.
>
> diff --git a/lib/CodeGen/BranchFolding.cpp b/lib/CodeGen/BranchFolding.cpp
> index 6080349..df5cac5 100644
> --- a/lib/CodeGen/BranchFolding.cpp
> +++ b/lib/CodeGen/BranchFolding.cpp
> @@ -780,7 +780,7 @@ removeMMOsFromMemoryOperations(MachineBasicBlock::iterator MBBIStartPos,
> assert(MBBICommon->isIdenticalTo(&*MBBI) && "Expected matching MIIs!");
>
> if (MBBICommon->mayLoad() || MBBICommon->mayStore())
> - MBBICommon->setMemRefs(MBBI->mergeMemRefsWith(*MBBI));
> + MBBICommon->setMemRefs(MBBICommon->mergeMemRefsWith(*MBBI));
>
> ++MBBI;
> ++MBBICommon;
> diff --git a/test/CodeGen/AArch64/branch-folder-merge-mmos.ll b/test/CodeGen/AArch64/branch-folder-merge-mmos.ll
> new file mode 100644
> index 0000000..1c79e60
> --- /dev/null
> +++ b/test/CodeGen/AArch64/branch-folder-merge-mmos.ll
> @@ -0,0 +1,44 @@
> +; RUN: llc -march=aarch64 -mtriple=aarch64-none-linux-gnu -stop-after branch-folder -o /dev/null < %s | FileCheck %s
This doesn’t make much sense to me - why the output is /dev/null? I’m surprised that could work at all. I guess it should be “-o -“, so that output would be in stdout, which then will be piped to FileCheck.
> +target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
> +
> +; Function Attrs: norecurse nounwind
> +define void @test(float* nocapture %test_arr) #0 {
Why does the test need this function? We don’t check anything in it, so I suppose it can be completely removed.
> +entry:
> + %0 = load float, float* %test_arr, align 4
> + %arrayidx1 = getelementptr inbounds float, float* %test_arr, i64 1
> + %1 = load float, float* %arrayidx1, align 4
> + %sub = fsub float %0, %1
> + store float %sub, float* %test_arr, align 4
> + ret void
> +}
> +
> +; Function Attrs: norecurse nounwind
> +define void @foo(i32 %a, i32 %b, float* nocapture %foo_arr) #0 {
It is a good practice to use "CHECK-LABEL: @foo” in the test. This way, if the test contains several functions, CHECK directives are guaranteed to work in the intended scope of the given function (they work in between CHECK-LABEL directives).
> +; CHECK: (load 4 from %ir.arrayidx1.{{i[1-2]}}), (load 4 from %ir.arrayidx1.{{i[1-2]}})
> +entry:
> + %cmp = icmp sgt i32 %a, 0
> + br i1 %cmp, label %if.then, label %if.end
> +
> +if.then: ; preds = %entry
> + %0 = load float, float* %foo_arr, align 4
> + %arrayidx1.i1 = getelementptr inbounds float, float* %foo_arr, i64 1
> + %1 = load float, float* %arrayidx1.i1, align 4
> + %sub.i = fsub float %0, %1
> + store float %sub.i, float* %foo_arr, align 4
> + br label %if.end3
> +
> +if.end: ; preds = %entry
> + %cmp1 = icmp sgt i32 %b, 0
> + br i1 %cmp1, label %if.then2, label %if.end3
> +
> +if.then2: ; preds = %if.end
> + %2 = load float, float* %foo_arr, align 4
> + %arrayidx1.i2 = getelementptr inbounds float, float* %foo_arr, i64 1
Michael
>
> Junmo.
>
> -----Original Message-----
> From: Benjamin Kramer [mailto:benny.kra at gmail.com <mailto:benny.kra at gmail.com>]
> Sent: Sunday, January 10, 2016 4:57 AM
> To: Junmo Park
> Cc: Chandler Carruth via llvm-commits
> Subject: Re: [llvm] r257253 - [BranchFolding] Set correct mem refs
>
> On Sat, Jan 9, 2016 at 8:30 AM, Junmo Park via llvm-commits
> <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>> Author: flyingforyou
>> Date: Sat Jan 9 01:30:13 2016
>> New Revision: 257253
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=257253&view=rev <http://llvm.org/viewvc/llvm-project?rev=257253&view=rev>
>> Log:
>> [BranchFolding] Set correct mem refs
>>
>> Merge MBBICommon and MBBI's MMOs.
>>
>> Differential Revision: http://reviews.llvm.org/D15990 <http://reviews.llvm.org/D15990>
>>
>> Added:
>> llvm/trunk/test/CodeGen/AArch64/branch-folder-merge-mmos.ll
>> Modified:
>> llvm/trunk/lib/CodeGen/BranchFolding.cpp
>>
>> Modified: llvm/trunk/lib/CodeGen/BranchFolding.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/BranchFolding.cpp?rev=257253&r1=257252&r2=257253&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/BranchFolding.cpp?rev=257253&r1=257252&r2=257253&view=diff>
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/BranchFolding.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/BranchFolding.cpp Sat Jan 9 01:30:13 2016
>> @@ -780,7 +780,7 @@ removeMMOsFromMemoryOperations(MachineBa
>> assert(MBBICommon->isIdenticalTo(&*MBBI) && "Expected matching MIIs!");
>>
>> if (MBBICommon->mayLoad() || MBBICommon->mayStore())
>> - MBBICommon->setMemRefs(MBBI->mergeMemRefsWith(*MBBI));
>> + MBBICommon->setMemRefs(MBBICommon->mergeMemRefsWith(*MBBI));
>>
>> ++MBBI;
>> ++MBBICommon;
>>
>> Added: llvm/trunk/test/CodeGen/AArch64/branch-folder-merge-mmos.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AArch64/branch-folder-merge-mmos.ll?rev=257253&view=auto <http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AArch64/branch-folder-merge-mmos.ll?rev=257253&view=auto>
>> ==============================================================================
>> --- llvm/trunk/test/CodeGen/AArch64/branch-folder-merge-mmos.ll (added)
>> +++ llvm/trunk/test/CodeGen/AArch64/branch-folder-merge-mmos.ll Sat Jan 9 01:30:13 2016
>> @@ -0,0 +1,53 @@
>> +; RUN: opt < %s -O3 | llc -march=aarch64 -mtriple=aarch64-none-linux-gnu -stop-after branch-folder -o /dev/null | FileCheck %s
>
> Why are you running opt -O3 on every run instead of just committing
> the optimized IR? Running the O3 pipeline can cause the test to fail
> when IR optimization passes change.
>
> - Ben
>
>> +target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
>> +
>> +; Function Attrs: nounwind
>> +define void @test(float* %test_arr) #0 {
>> +entry:
>> + %test_arr.addr = alloca float*, align 8
>> + store float* %test_arr, float** %test_arr.addr, align 8
>> + %0 = load float*, float** %test_arr.addr, align 8
>> + %arrayidx = getelementptr inbounds float, float* %0, i64 0
>> + %1 = load float, float* %arrayidx, align 4
>> + %2 = load float*, float** %test_arr.addr, align 8
>> + %arrayidx1 = getelementptr inbounds float, float* %2, i64 1
>> + %3 = load float, float* %arrayidx1, align 4
>> + %sub = fsub float %1, %3
>> + %4 = load float*, float** %test_arr.addr, align 8
>> + %arrayidx2 = getelementptr inbounds float, float* %4, i64 0
>> + store float %sub, float* %arrayidx2, align 4
>> + ret void
>> +}
>> +
>> +; Function Attrs: nounwind
>> +define void @foo(i32 %a, i32 %b, float* %foo_arr) #0 {
>> +; CHECK: (load 4 from %ir.arrayidx1.i2), (load 4 from %ir.arrayidx1.i)
>> +entry:
>> + %a.addr = alloca i32, align 4
>> + %b.addr = alloca i32, align 4
>> + %foo_arr.addr = alloca float*, align 8
>> + store i32 %a, i32* %a.addr, align 4
>> + store i32 %b, i32* %b.addr, align 4
>> + store float* %foo_arr, float** %foo_arr.addr, align 8
>> + %0 = load i32, i32* %a.addr, align 4
>> + %cmp = icmp sgt i32 %0, 0
>> + br i1 %cmp, label %if.then, label %if.end
>> +
>> +if.then: ; preds = %entry
>> + %1 = load float*, float** %foo_arr.addr, align 8
>> + call void @test(float* %1)
>> + br label %if.end3
>> +
>> +if.end: ; preds = %entry
>> + %2 = load i32, i32* %b.addr, align 4
>> + %cmp1 = icmp sgt i32 %2, 0
>> + br i1 %cmp1, label %if.then2, label %if.end3
>> +
>> +if.then2: ; preds = %if.end
>> + %3 = load float*, float** %foo_arr.addr, align 8
>> + call void @test(float* %3)
>> + br label %if.end3
>> +
>> +if.end3: ; preds = %if.then, %if.then2, %if.end
>> + ret void
>> +}
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> <diff.txt>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160110/772b9a51/attachment.html>
More information about the llvm-commits
mailing list