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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/examples/IRTransforms/SimplifyCFG.cpp:15
+// TODOs
+//  * Hook up pass to the new pass manager.
+//  * Preserve LoopInfo.
----------------
andwar wrote:
> 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.
I'd prefer to get a first version in sooner rather than later and do the new PM support as follow-up.  I've removed the NewPM stuff in the meantime.


================
Comment at: llvm/examples/IRTransforms/SimplifyCFG.cpp:372-374
+FunctionPass *llvm::createSimplifyCFGPass() {
+  return new SimplifyCFGLegacyPass();
+}
----------------
andwar wrote:
> AFAIK, this is neither used nor needed.
That's a leftover from living in lib/Transforms. I've had it in the passmanagerbuilder as well.


================
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:
----------------
andwar wrote:
> The following line suggests that `BB_2` is always `bb.2`. You don't need a regex here.
That's what the autogeneration script generates. I agree it's not ideal and we should fix it there.


================
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:
----------------
andwar wrote:
> The following line suggests that `BB_3` is always `bb.3`.
Same as earlier, this should be fixed in the generation script.


================
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.
----------------
andwar wrote:
> Won't the default version (i.e. `v1`) invalidate the DominatorTree? (i.e. it won't use DomTreeUpdater).
Yes, this is just relevant for the DomTreeUpdater version. It's tested now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69416