[PATCH] D86844: [LoopDeletion] Allows deletion of possibly infinite side-effect free loops

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!



================
Comment at: clang/test/Misc/loop-opt-setup.c:26
 
 // Check br i1 to make sure the loop is gone, there will still be a label branch for the infinite loop.
 // CHECK-LABEL: Helper
----------------
fhahn wrote:
> This comment needs updating, there's no loop there now. Or better, add a run-line with a C standard version that does not have the forward progress guarantee, e.g. `-std=c99` and one with an explicit standard that has it and have different check lines for the 2 cases.
Can you also explain the C99 case in the comment?


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:621
       }
     }
 
----------------
atmnpatel wrote:
> nikic wrote:
> > Unrelated, but why do these updates happen before the branch from preheader to exit is added in IR? Shouldn't it be the other way around according to the DTU contract?
> Isn't that branch added on line 602? My understanding was the changes on line 640 onwards are for removing, not introducing a branch.
> Shouldn't it be the other way around according to the DTU contract?

Yes I think so. Perhaps a missing case in the DTU validator. Probably good to check/fix separately.


================
Comment at: llvm/test/Transforms/LoopDeletion/mustprogress.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --check-attributes
+; RUN: opt < %s -loop-deletion -S | FileCheck %s
+
----------------
probably good to also add `-verify-dom-info`


================
Comment at: llvm/test/Transforms/LoopDeletion/mustprogress.ll:5
+;; Original C Code:
+;;  void unknown_tripcount_mustprogress_attr_mustprogress_loopmd(int a, int b) {
+;;    for (; a < b;) ;
----------------
FWIW, I don't think the C code doesn't add much, but I don't have any strong feelings about it. The IR is what is key and it's really small.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86844