[test] Fix FullUnroll.ll

[aeubanks]

Summary

I believe the intention of this test added in
https://reviews.llvm.org/D71687 was to test LoopFullUnrollPass with
clang's -fno-unroll-loops, not its interaction with optnone. Loop
unrolling passes don't run under optnone/-O0.

Also added back unintentionally removed -disable-loop-unrolling from
https://reviews.llvm.org/D85578.

[echristo]

The code has a pragma to turn on loop unrolling. I actually did want to check with optnone.

[aeubanks]

In D86485#2234668, @echristo wrote:

The code has a pragma to turn on loop unrolling. I actually did want to check with optnone.

But the pragmas don't have an effect for -O0 in both the legacy and new PMs. And the legacy PM skipped loop unrolling on all optnone functions: https://github.com/llvm/llvm-project/blob/c1d25e9a82554aa580b3cc0b97fc5c7db8164042/llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp#L1231. So the NPM pass probably also should.

$ cat /tmp/a.c
void b();

void a() {
#pragma clang loop unroll(full)
        for (int i = 0; i < 50; i++) {
                b();
        }
}
$ ./build_debug/bin/clang -S -emit-llvm /tmp/a.c -O0 -o -
... not unrolled LLVM IR
$ ./build_debug/bin/clang -S -emit-llvm /tmp/a.c -O0 -o - -fexperimental-new-pass-manager
... not unrolled LLVM IR
$ ./build_debug/bin/clang -S -emit-llvm /tmp/a.c -O1 -o -
... unrolled LLVM IR
$ ./build_debug/bin/clang -S -emit-llvm /tmp/a.c -O1 -o - -fexperimental-new-pass-manager
... unrolled LLVM IR

[aeubanks]

ping?

[echristo]

I don't think your statement matches what I was seeing. I may have
constructed the testcase poorly, but there were differences being shown.
Also keep in mind that your clang run is testing what I've already fixed. :)

Happy to work with you on a testcase here, but I don't have time this week.

[rnk]

Here is the sequence of what happened as I understand it:

• D71687 is intended to fix the full loop unrolling pragma with the NPM at -O1. The accompanying clang test covers this.
• D71687 also adds an LLVM IR test. It is an integration test: it started from unoptimized IR and ran the full -O1 NPM pipeline. The IR happens to contain optnone. Maybe that was unintentional, since one used to be able to run clang foo.c -emit-llvm -o - | opt -O1 -S and get optimized IR. Now clang applies the optnone attribute, and that no longer works.
• D85578 made the IR test more targeted: now it only runs the full unrolling pass instead of the O1 pipeline.

There is value in integration tests, so maybe the thing to do is to bring back the IR integration test in addition to this unit test. The unit test seems correct to me. If optnone is present, I would not expect the loop unroll metadata to be honored.

In D86485#2234668, @echristo wrote:

The code has a pragma to turn on loop unrolling. I actually did want to check with optnone.

Is that the intended behavior? Is clang supposed to fully unroll such a loop at -O0? I would expect it not to. The documentation doesn't say anything:
http://clang.llvm.org/docs/LanguageExtensions.html#extensions-for-loop-hint-optimizations

[echristo]

In D86485#2277714, @rnk wrote:

Here is the sequence of what happened as I understand it:

• D71687 is intended to fix the full loop unrolling pragma with the NPM at -O1. The accompanying clang test covers this.
• D71687 also adds an LLVM IR test. It is an integration test: it started from unoptimized IR and ran the full -O1 NPM pipeline. The IR happens to contain optnone. Maybe that was unintentional, since one used to be able to run clang foo.c -emit-llvm -o - | opt -O1 -S and get optimized IR. Now clang applies the optnone attribute, and that no longer works.
• D85578 made the IR test more targeted: now it only runs the full unrolling pass instead of the O1 pipeline.

There is value in integration tests, so maybe the thing to do is to bring back the IR integration test in addition to this unit test. The unit test seems correct to me. If optnone is present, I would not expect the loop unroll metadata to be honored.

I think I did, but I also don't feel strongly about it one way or another.

In D86485#2234668, @echristo wrote:

The code has a pragma to turn on loop unrolling. I actually did want to check with optnone.

Is that the intended behavior? Is clang supposed to fully unroll such a loop at -O0? I would expect it not to. The documentation doesn't say anything:
http://clang.llvm.org/docs/LanguageExtensions.html#extensions-for-loop-hint-optimizations

*shrug* :)

I thought so, but I'm ok not. As long as we can continue to test it's fine. I'll ack this and just hope we're not removing a testing expectation at the moment. :)

llvm/test/Transforms/LoopUnroll/FullUnroll.ll

-; RUN: opt -passes='loop-unroll-full' -disable-verify --mtriple x86_64-pc-linux-gnu -S -o - %s | FileCheck %s
+; RUN: opt -passes='loop-unroll-full' -disable-verify -disable-loop-unrolling=true --mtriple x86_64-pc-linux-gnu -S -o - %s | FileCheck %s
 
 ; This checks that the loop full unroller will fire in the new pass manager
 ; when forced via #pragma in the source (or annotation in the code).
 
 ; Completely unroll the inner loop
 ; CHECK-LABEL: @foo
 ; CHECK: br i1
 ; CHECK-NOT: br i1
bb10:                                             ; preds = %bb10, %bb7.preheader
   %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
   %exitcond.not = icmp eq i64 %indvars.iv.next, 5
   br i1 %exitcond.not, label %bb3.loopexit, label %bb10, !llvm.loop !1
 
 bb24:                                             ; preds = %bb3.loopexit
   ret void
 }
 
-attributes #0 = { noinline nounwind optnone uwtable }
+attributes #0 = { nounwind uwtable }
 
 !llvm.module.flags = !{!0}
 
 !0 = !{i32 1, !"wchar_size", i32 4}
 !1 = distinct !{!1, !2}
 !2 = !{!"llvm.loop.unroll.full"}