[PATCH] D50593: ConstantMerge: merge common initial sequences

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya added a comment.

Have you tested this patch with some benchmarks or an open source project? What is the impact on compilation time?



================
Comment at: lib/Transforms/IPO/ConstantMerge.cpp:111
+const char *toString(CommonSequence S) {
+  switch (S) {
+  case CommonSequence::Same:
----------------
if-else instead of switch?


================
Comment at: lib/Transforms/IPO/ConstantMerge.cpp:181
+      CommonInitialSequenceRewrite[*Smaller] = *Bigger;
+      goto matched;
+
----------------
Do we really need a goto here?


================
Comment at: lib/Transforms/IPO/ConstantMerge.cpp:183
+
+    different:
+      continue;
----------------
I think we should split the nested loop to separate function that may help remove goto.


================
Comment at: test/Transforms/ConstantMerge/initial-match.ll:1
+; RUN: opt -constmerge -S < %s | FileCheck %s
+
----------------
Can we reduce the size of this test case and maybe make multiple functions.


Repository:
  rL LLVM

https://reviews.llvm.org/D50593