[PATCH] D50593: ConstantMerge: merge common initial sequences

[JF Bastien via Phabricator via llvm-commits]

jfb added a comment.

In https://reviews.llvm.org/D50593#1198844, @hiraditya wrote:

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


Yes! I did so over the weekend and then discarded my results on Monday because I'm Smart. I'll re-run a stage 2 bootstrap before / after.



================
Comment at: lib/Transforms/IPO/ConstantMerge.cpp:111
+const char *toString(CommonSequence S) {
+  switch (S) {
+  case CommonSequence::Same:
----------------
hiraditya wrote:
> if-else instead of switch?
This is on purpose: a `switch` without `default` will cause a diagnostic if a new entry is added to the `enum class` but isn't handled here. 


================
Comment at: lib/Transforms/IPO/ConstantMerge.cpp:181
+      CommonInitialSequenceRewrite[*Smaller] = *Bigger;
+      goto matched;
+
----------------
hiraditya wrote:
> Do we really need a goto here?
I like `goto` to break out of loop nests. :-)


================
Comment at: lib/Transforms/IPO/ConstantMerge.cpp:183
+
+    different:
+      continue;
----------------
xbolva00 wrote:
> hiraditya wrote:
> > I think we should split the nested loop to separate function that may help remove goto.
> Yes, please split it.
Done... but I still like `goto` for loop nests.


================
Comment at: lib/Transforms/IPO/ConstantMerge.cpp:379
+    // Replacements where initializers have a common initial sequence.
+    for (auto I = CommonInitialSequenceRewrite.begin(),
+              E = CommonInitialSequenceRewrite.end();
----------------
xbolva00 wrote:
> cannot we use for (for &I : CommonInitialSequenceRewrite)?
`erase` needs an iterator, and I was previously using another container type. Now that I use `MapVector` it's better to do a first-pass `erase_if` and then do the rest of the loop with a range-based for, so I updated the code to do so.


================
Comment at: test/Transforms/ConstantMerge/initial-match.ll:1
+; RUN: opt -constmerge -S < %s | FileCheck %s
+
----------------
hiraditya wrote:
> Can we reduce the size of this test case and maybe make multiple functions.
I could, but one of the goals is to make sure things work through multiple iterations of the outer `while` (because an earlier implementation I had didn't). If I break it up then you end up with some single-iteration things. These (if implemented improperly) would break as tested, but wouldn't break in single-iteration.

In particular, `removeDeadConstantUsers` and `eraseFromParent` are trip hazards because they can invalidate values we discover. For example, you might be tempted to cache initial sequence matches found from one iteration to another (so you don't re-discover them every iteration), but that would break if `removeDeadConstantUsers` kicks in.


Repository:
  rL LLVM

https://reviews.llvm.org/D50593