[PATCH] D69416: [Examples] Add IRTransformations directory to examples.
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 6 04:14:16 PST 2019
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
More information about the llvm-commits
mailing list