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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:50
 
-#define DEBUG_TYPE "loop-unroll"
+#define DEBUG_TYPE "loop-peel"
 
----------------
fhahn wrote:
> Meinersbur wrote:
> > fhahn wrote:
> > > 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 ;)
> > If you want both, use `-debug-only=loop-unroll,loop-peel`. For discoverability one can emit everything using just `-debug`. Ultimately, every change can be surprising.
> > 
> > `DEBUG_TYPE` is for debugging, not an intentional behavioral change (in debug builds). I agree this is a somewhat grey area for a NFC(I) patch, but in the strict interpretation assertion also may change behavior and we change those regularly in refactoring NFC changes. I don't think we have a policy for this case.
> > If you want both, use -debug-only=loop-unroll,loop-peel. For discoverability one can emit everything using just -debug. Ultimately, every change can be surprising.
> 
> Yep, I can't think of a better alternative either.
> 
> > DEBUG_TYPE is for debugging, not an intentional behavioral change (in debug builds). I agree this is a somewhat grey area for a NFC(I) patch, but in the strict interpretation assertion also may change behavior and we change those regularly in refactoring NFC changes. I don't think we have a policy for this case.
> 
> Sure, and the distinction is not very important IMO. But technically a new assertion should never trigger (and nothing is observable), but changing DEBUG_TYPE changes observable behavior in debug builds.
`shouldReturnTrue()` in `assert(shouldReturnTrue())` might print to `dbgs()` and increase statistics counters and therefore also changes observable behavior.

We could just remove the `[NFC]` tag from the title, it's a gray area anyway.


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

https://reviews.llvm.org/D83056