[PATCH] D99205: Add jump-threading optimization for deterministic finite automata

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DFAJumpThreading.cpp:381
+
+  virtual ~MainSwitch() = default;
+
----------------
alexey.zhikhar wrote:
> ChuanqiXu wrote:
> > alexey.zhikhar wrote:
> > > ChuanqiXu wrote:
> > > > Does it necessary to mark `~MainSwitch` as virtual? Since there is no derived class from MainSwitch.
> > > Classes might derive from `MainSwitch` in the future, it's a good practice to declare destructors virtual.
> > So it may be better to add `virtual ~MainSwitch` in the future.
> Let's see if Sjoerd @SjoerdMeijer has an opinion here.
No strong opinion. Looks fine as it is.


================
Comment at: llvm/test/Transforms/DFAJumpThreading/negative.ll:2
+; RUN: opt -dfa-jump-threading -pass-remarks-missed='dfa-jump-threading' -pass-remarks-output=%t -disable-output %s
+; RUN: FileCheck --input-file %t --check-prefix REMARK %s
+
----------------
Perhaps this works, but thought this should be:

  --check-prefix=REMARK 


================
Comment at: llvm/test/Transforms/DFAJumpThreading/negative.ll:185
+  ret i32 0
+}
----------------
I don't think we want to jump thread a function that has a `minsize` attribute? For example:

  define i32 @negative5( ) minsize {

Need a test for that?


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

https://reviews.llvm.org/D99205