[PATCH] D138232: Code for calculating number of loop peels based on phi invariance

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

General notion: if your claim is that the existing code isn't handling some case, then you need to demonstrate it in a test. Steps are the following:

- Create a test, run `utils/update_test_checks.py` on it with existing implementation, and commit it;
- With your change, run this script again and re-generate checks so that they clearly show the difference;

Without it, it's hard to say how much better it became.

Also think if it can be split into several patches, some being NFC (e.g. factor out this logic into a separate class), which I'm generally fine with, and semantically meaningful functionality changes.



================
Comment at: llvm/test/Transforms/LoopUnroll/peel-loop-phi-analysis.ll:6
+
+; RUN: opt < %s -S -loop-unroll | FileCheck %s
+; RUN: opt < %s -S -passes=loop-unroll | FileCheck %s
----------------
This is in the process of deprecation, pls only use new PM.


================
Comment at: llvm/test/Transforms/LoopUnroll/peel-loop-phi-analysis.ll:16
+; Check that phi analysis can handle a cast.
+define void @_Z8castTestv() {
+; The phis become invariant through the chain of phis, with a unary
----------------
I would strongly prefer auto-generated full checks over anything else. Please use `utils/update_test_checks.py`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138232