[PATCH] D69416: [Examples] Add IRTransformations directory to examples.

[Andrzej Warzynski via Phabricator via llvm-commits]

andwar added a comment.

This is great stuff, thank you for doing this!

1. Why not test with all versions (`v1`, `v2` and `v3`) in all tests? Does the output change?
2. AFAIK there are no official guidelines on this (and both approaches are used in practice), but I'd rename `SimplifyCFGLegacyPass` to `SimplifyCFGLegacy` (fewer characters, meaning still clear). But please use the one you prefer the most.
3. I'd prefer to see this fully working with the new PM or no new PM support at all. Otherwise it can be confusing.



================
Comment at: llvm/examples/IRTransforms/SimplifyCFG.cpp:10
+//
+// This file implements implements the CFG simplifications presented as part of
+// the 'Getting Started With LLVM: Basics' tutorial at the US LLVM Developers
----------------
1. Could you add a quick summary of your passes (and the variants)? Also, since this a tutorial - maybe also worth mentioning the tests? They are an integral part of this, but this could be non-obvious for beginners.
2. "implements" twice
3. Definitely worth adding a link to the video (once it's uploaded)


================
Comment at: llvm/examples/IRTransforms/SimplifyCFG.cpp:15
+// TODOs
+//  * Hook up pass to the new pass manager.
+//  * Preserve LoopInfo.
----------------
Why not do it now? AFAIK it's been agreed that the new PM is to become the default.

If you prefer to leave it for later then I wouldn't be adding the new PM implementation at all. It's currently not used and not tested.


================
Comment at: llvm/examples/IRTransforms/SimplifyCFG.cpp:123
+// which *does not* preserve the dominator tree.
+static bool eliminateCondBranches_v1(Function &F) {
+  bool Changed = false;
----------------
[nit] This is (together with v2 and v3) your least commented function. 


================
Comment at: llvm/examples/IRTransforms/SimplifyCFG.cpp:372-374
+FunctionPass *llvm::createSimplifyCFGPass() {
+  return new SimplifyCFGLegacyPass();
+}
----------------
AFAIK, this is neither used nor needed.


================
Comment at: llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg2-dead-block-order.ll:44
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[BB_2:%.*]]
+; CHECK:       bb.2:
----------------
The following line suggests that `BB_2` is always `bb.2`. You don't need a regex here.


================
Comment at: llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg2-dead-block-order.ll:91
+  ret i32 1
+
+
----------------
[nit] extra line


================
Comment at: llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg3-phis.ll:23
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[BB_3:%.*]]
+; CHECK:       bb.3:
----------------
The following line suggests that `BB_3` is always `bb.3`.


================
Comment at: llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg4-multiple-duplicate-cfg-updates.ll:4
+
+; Check that we do not crash when we remove edges multiple times in
+; the DomTreeUpdater.
----------------
Won't the default version (i.e. `v1`) invalidate the DominatorTree? (i.e. it won't use DomTreeUpdater).


================
Comment at: llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg6-dead-self-loop.ll:18
+bb.1:
+  ;%p = phi i32 [ 0, %bb.1]
+  br label %bb.1
----------------
DELETEME


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69416