[dataflow] Avoid copying environment

[sammccall]

Summary

This appears to be just an accidental copy rather than move from a scratch
variable.
As well as doing redundant work, these copies introduce extra SAT variables
which make debugging harder (each Enviroment has a unique FC token).

Example flow condition before:

(B0:1 = V15)
(B1:1 = V8)
(B2:1 = V10)
(B3:1 = (V4 & (!V7 => V6)))
(V10 = (B3:1 & !V7))
(V12 = B1:1)
(V13 = B2:1)
(V15 = (V12 | V13))
(V3 = V2)
(V4 = V3)
(V8 = (B3:1 & !!V7))
B0:1
V2

after:

(B0:1 = (V9 | V10))
(B1:1 = (B3:1 & !!V6))
(B2:1 = (B3:1 & !V6))
(B3:1 = (V3 & (!V6 => V5)))
(V10 = B2:1)
(V3 = V2)
(V9 = B1:1)
B0:1
V2

(with labelling from D153488)

There are also some more copies that can be avoided here (when multiple blocks
without terminating statements are joined), but they're less trivial, so I'll
put those in another patch.

[xazax.hun]

This sounds extremely error-prone to me. In case copying the analysis state has side effects like this, I would argue we want such operations to be really explicit. What do you think?

[ymandel]

In D153491#4443704, @xazax.hun wrote:

This sounds extremely error-prone to me. In case copying the analysis state has side effects like this, I would argue we want such operations to be really explicit. What do you think?

Can you expand on this concern? Are you referring to the removal of the unintended copy (ie. this patch), or raising a concern about the how the underlying system handles copies altogether?

[sammccall]

In D153491#4445051, @ymandel wrote:

In D153491#4443704, @xazax.hun wrote:

This sounds extremely error-prone to me. In case copying the analysis state has side effects like this, I would argue we want such operations to be really explicit. What do you think?

Can you expand on this concern? Are you referring to the removal of the unintended copy (ie. this patch), or raising a concern about the how the underlying system handles copies altogether?

FWIW I would prefer if we can call this operation clone() or fork() or something rather than have it be the copy constructor.

It is a kind of copy but between it being quite expensive, this effect on the SAT system, and intentional copies being quite an important and deliberate thing, making it a bit verbose seems justified.

I'll send a patch for this...

[gribozavr2]

LGTM

[xazax.hun]

In D153491#4445051, @ymandel wrote:

In D153491#4443704, @xazax.hun wrote:

This sounds extremely error-prone to me. In case copying the analysis state has side effects like this, I would argue we want such operations to be really explicit. What do you think?

Can you expand on this concern? Are you referring to the removal of the unintended copy (ie. this patch), or raising a concern about the how the underlying system handles copies altogether?

The latter, https://reviews.llvm.org/D153674 should address my concerns.

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

for (const CFGBlock *Pred : Preds) {
     }
   }
   if (!MaybeState) {
     // FIXME: Consider passing `Block` to `Analysis.typeErasedInitialElement()`
     // to enable building analyses like computation of dominators that
     // initialize the state of each basic block differently.
     MaybeState.emplace(Analysis.typeErasedInitialElement(), AC.InitEnv);
   }
-  return *MaybeState;
+  return std::move(*MaybeState);
 }
 
 /// Built-in transfer function for `CFGStmt`.
 static void
 builtinTransferStatement(const CFGStmt &Elt,
                          TypeErasedDataflowAnalysisState &InputState,
                          AnalysisContext &AC) {
   const Stmt *S = Elt.getStmt();