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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:19
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SetVector.h"
+#include "llvm/ADT/SmallPtrSet.h"
----------------
It seems like some of the new includes are not being used? Are they all required?


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:36
-#include "llvm/Support/CommandLine.h"
-#include "llvm/Support/Debug.h"
-#include "llvm/Support/raw_ostream.h"
----------------
I think stuff in Debug.h/CommandLine.h is directly used in this file (e.g. cl::opt is defined in CommandLine.h I think). Why are those removed? (I did not check all other includes, but suspect there are more that should not be removed)


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:626
+
+  // Get Target Specifc Values
+  TTI.getPeelingPreferences(L, SE, PP);
----------------
nit: usually 'full' sentences are used for comments, so it might be good to add a period at the end and not capitalize words in the middle of the sentence, both there and in the other comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:50
 
-#define DEBUG_TYPE "loop-unroll"
+#define DEBUG_TYPE "loop-peel"
 
----------------
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.


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

https://reviews.llvm.org/D83056