[PATCH] D86673: [StackColoring] Conservatively merge Variable in catch(Variable)

[Than McIntosh via Phabricator via llvm-commits]

thanm added a comment.

In your commit message. you refer to "Conservatively merge &(&Variable) for catch(Variable)" -- this seems like a problematic choice of words/terminology. StackColoring operates on slots, not on addresses of slots.

Also "there is a potion" => "there is an option"



================
Comment at: llvm/lib/CodeGen/StackColoring.cpp:437
 
+  /// Record the FI slots for which may write memory.
+  BitVector StoreSlots;
----------------
"for which may write memory" is awkward wording. How about "Record the FI slots referenced by a 'may write to memory' instruction?


================
Comment at: llvm/lib/CodeGen/StackColoring.cpp:696
           }
+          // There is a bug for using LifetimeStartOnFirstUse in win32.
+          // class Type1 {
----------------
I think it would be better to append this comment to the large block at the start of the file (from lines 103-375), since that's currently the place for discussions about how the algorithm works and the various limitations. You can then refer back up to it here. 


================
Comment at: llvm/lib/CodeGen/StackColoring.cpp:708
+          // }
+          // For variable X in catch(X), we put &(&X) into ConservativeSlots
+          // to prevent using LifetimeStartOnFirstUse. Because &(&X) may merged
----------------
This doesn't make sense to me. ConservativeSlots contains slots, not addresses of slots -- I think it would be clearer just to say "we put X into ConservativeSlots".


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86673/new/

https://reviews.llvm.org/D86673