[PATCH] D83056: [NFC] Separate the Loop Peeling Utilities from the Loop Unrolling Utilities

Tue Jul 7 15:05:17 PDT 2020

fhahn added inline comments.

================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:50

-#define DEBUG_TYPE "loop-unroll"
+#define DEBUG_TYPE "loop-peel"

----------------
sidbav wrote:
> Meinersbur wrote:
> > fhahn wrote:
> > > I am not sure about this change. Currently peeling is integrated in loop-unroll and remarks/debug can be filtered by loop-unroll, but now we will generate remarks for `loop-unroll` and `loop-peel` when running `-loop-unroll`.
> > Isn't it actually better since you can now filter `-debug-only=loop-unroll`, respectively `-debug-only=loop-peel` depending on what you want to look at?
> > 
> > Note: `-Rpass=` remarks use the pass name, not `DEBUG_TYPE`.
> I also agree with @Meinersbur, having them separate is better. Additionally, in the case that the developer wants to look at both unrolling and peeling at the same time, they can specify `debug-only=loop-unroll,loop-peel`.
> Isn't it actually better since you can now filter -debug-only=loop-unroll, respectively -debug-only=loop-peel depending on what you want to look at?

I'd say it depends. Personally I find it mostly makes things less discoverable for newcomers. I can see how it might be surprising if a user wants to ask for debug output of the LoopUnroll pass and then the pass makes changes but doesn't display the debug output. It's certainly not a new problem though and not a blocker. I think it means that the patch changes behavior though ;)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83056