[NFC-for-now] Fix LocalCSE bug in ignoring traps (#8385)

The code did "trap = false; scan()", but that is wrong, as it wants to
remove the trap after the scan. Reorder and just use a ShallowEffectAnalyzer.

But this logic bug was not actually allowing different behavior, see the
detailed comment - we have no effects atm that can distinguish the two
cases. If we add new effects, we might, though, so this is worth doing.

Author: kripken

src/passes/LocalCSE.cpp

@@ -465,7 +465,9 @@ struct Checker
     // Given the current expression, see what it invalidates of the currently-
     // hashed expressions, if there are any.
     if (!activeOriginals.empty()) {
-      EffectAnalyzer effects(options, *getModule());
+      // We only need to visit this node itself, as we have already visited its
+      // children by the time we get here.
+      ShallowEffectAnalyzer effects(options, *getModule(), curr);
       // We can ignore traps here:
       //
       //  (ORIGINAL)
@@ -475,10 +477,15 @@ struct Checker
       // We are some code in between an original and a copy of it, and we are
       // trying to turn COPY into a local.get of a value that we stash at the
       // original. If |curr| traps then we simply don't reach the copy anyhow.
+      //
+      // Note, however, that this is really for future use, as atm this does not
+      // help: the only effect a trap can interact with is a set of global state
+      // (the trap could prevent that writing, which would be noticeable) - but
+      // LocalCSE does not deduplicate expressions with such effects, as they
+      // must happen twice. That is, removing trapping from (curr) in the
+      // example above has no effect as (ORIGINAL) never has global write
+      // effects.
       effects.trap = false;
-      // We only need to visit this node itself, as we have already visited its
-      // children by the time we get here.
-      effects.visit(curr);
 
       auto idempotent = isIdempotent(curr, *getModule());