[PATCH] D11629: APValues and Constants and MaterializedTemporaryExpr need to have stable maps

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: lib/AST/ASTContext.cpp:8557
@@ +8556,3 @@
+  if (MayCreate) {
+    auto *MTVI = &MaterializedTemporaryValues[E];
+    if (*MTVI == nullptr) {
----------------
Maybe use a reference here rather than a pointer.

================
Comment at: lib/AST/ASTContext.cpp:8559-8560
@@ +8558,4 @@
+    if (*MTVI == nullptr) {
+      *MTVI = BumpAlloc.Allocate<APValue>();
+      new (*MTVI) APValue();
+    }
----------------
More succinctly expressed as:

    MTVI = new (*this) APValue;

================
Comment at: lib/AST/ASTContext.cpp:8565-8566
@@ -8558,5 +8564,4 @@
 
-  llvm::DenseMap<const MaterializeTemporaryExpr *, APValue>::iterator I =
-      MaterializedTemporaryValues.find(E);
-  return I == MaterializedTemporaryValues.end() ? nullptr : &I->second;
+  auto I = MaterializedTemporaryValues.find(E);
+  return I == MaterializedTemporaryValues.end() ? nullptr : I->second;
 }
----------------
This is now just `MaterializedTemporaryValues.lookup(E)`

================
Comment at: lib/CodeGen/CodeGenModule.h:377
@@ -376,3 +376,3 @@
   llvm::DenseMap<const Decl*, llvm::GlobalVariable*> StaticLocalDeclGuardMap;
-  llvm::DenseMap<const Expr*, llvm::Constant *> MaterializedGlobalTemporaryMap;
+  std::map<const Expr*, llvm::Constant *> MaterializedGlobalTemporaryMap;
 
----------------
Can you fix this instead by not holding onto `Slot` in `CodeGenModule::GetAddrOfGlobalTemporary`? (Do the map lookup again at the end of the function.) Twice as many `DenseMap` lookups seems likely to be cheaper than switching to a `std::map`.


http://reviews.llvm.org/D11629