[PATCH] D71687: Fix full loop unrolling initialization in new pass manager

Chandler Carruth via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue May 12 23:57:48 PDT 2020


chandlerc added inline comments.


================
Comment at: clang/test/Misc/loop-opt-setup.c:12
+// Check br i1 to make sure that the loop is fully unrolled
 // CHECK-NOT: br i1
 
----------------
This is dead now that you have different prefixes...


================
Comment at: clang/test/Misc/loop-opt-setup.c:32-33
+
+// Check br i1 to make sure the loop is gone, there will still be a label branch for the infinite loop.
+// CHECK-NEWPM-NOT: br i1
+
----------------
But for which function?

I'd rework these to be more modern `CHECK-LABEL` and affirmative checks for the unrolling.


================
Comment at: llvm/test/Transforms/LoopUnroll/FullUnroll.ll:4-6
+; We don't end up deleting the loop, but we remove everything inside of it so checking for any
+; reasonable instruction from the original loop will work.
+; CHECK-NOT: br i1
----------------
echristo wrote:
> chandlerc wrote:
> > Make sure it is in the correct function at least, and maybe after the label for the loop header?
> Got it. There's not a lot of function left at the end:
> 
> define void @walrus() local_unnamed_addr #0 {
> entry:
>   br label %for.cond.preheader
> 
> for.cond.preheader:                               ; preds = %for.cond.preheader, %entry
>   br label %for.cond.preheader
> }
Then switch to generated `CHECK` lines?

The checks we have here will pass easily after incorrect edits that cause this test to not actually check what it intends to. I'd either check something that constructively shows unrolling or generate exact checks (if its small enough to be genuinely stable).


================
Comment at: llvm/test/Transforms/LoopUnroll/FullUnroll.ll:19-20
+
+; Function Attrs: noinline nounwind optnone uwtable
+define linkonce_odr dso_local void @_Z6Helperv() #1 comdat {
+entry:
----------------
echristo wrote:
> chandlerc wrote:
> > Nit, but minimize and clean this function up a touch?
> > 
> > At the least, removing all the target features seems valuable, and I'd give things stable names instead of numbered values.
> Got it. Most things have stable names, only a few temporaries have numbered names.
Sure, but even those will make future edits to this really frustrating with irrelevant diff, etc.

And this test case can still be minimized. As an example, the function is currently attributed as *optnone*. =/

I think you can just run this through the metarenamer pass?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71687/new/

https://reviews.llvm.org/D71687





More information about the llvm-commits mailing list