<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""></div><div class=""><br class=""><div class=""><div><blockquote type="cite" class=""><div class="">On Jun 24, 2015, at 11:02 AM, Pete Cooper <<a href="mailto:peter_cooper@apple.com" class="">peter_cooper@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class="Apple-interchange-newline">On Jun 23, 2015, at 6:05 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" class="">dexonsmith@apple.com</a>> wrote:<br class=""><br class=""><br class=""><blockquote type="cite" class="">On 2015-Jun-23, at 17:24, Pete Cooper <<a href="mailto:peter_cooper@apple.com" class="">peter_cooper@apple.com</a>> wrote:<br class=""><br class="">On to 0003 which is devirtualizing Constant::replaceUsesOfWithOnConstant.<br class=""><br class="">Similarly to 0002, I ended up changing this so that we have Constant::replaceUsesOfWithOnConstant and then all subclasses must implement replaceUsesOfWithOnConstantImpl.<br class=""><br class="">Given the way this code was structured, I ended up having to make replaceUsesOfWithOnConstantImpl return either a value to replace with, or nullptr if no replacement should be done.<br class=""></blockquote><br class="">New behaviour makes sense to me. At first blush I thought returning<br class="">`nullptr` was worse that returning `this`, but the way you've done it<br class="">makes the implementations way simpler, and I guess this pattern matches<br class="">lots of other code in LLVM.<br class=""><br class=""><blockquote type="cite" class="">Perhaps there’s a better name for it at this point?<br class=""></blockquote><br class="">Well, it's good that they're named after each other (foo and fooImpl).<br class=""><br class="">`replaceUsesOfWithOnConstant()` is definitely an eyeful, but it's<br class="">somewhat logical that it's based on `replaceUsesOfWith()`. But given<br class="">that there's only one user (`replaceUsesOfWith()`), this is probably<br class="">good timing to rename it if we ever do.<br class=""><br class="">We use `handleOperandChange()` in the `Metadata` hierarchy. Since this<br class="">*only* gets called during RAUW, I kind of like `handleOperandRAUW()`;<br class="">another straw man would be to match `Metadata`'s name exactly. The<br class="">"constant" in the name is redundant, since this is API for `Constant`.<br class=""></blockquote><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">You’re right about the renaming, especially with a single caller.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">I was thinking along the lines of ‘replaceUsesOfValue’ but its confusing as we’re replacing operands which is what the Constant is using, not users of the Constant. I prefer handleOperandChange, especially with it being the same as Metadata so i’ll go with that.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class="">I'm happy for you to just leave well-enough alone, too.<br class=""><br class=""><blockquote type="cite" class="">Constant:: replaceUsesOfWithOnConstant then does as expected and dispatches to the appropriate subclass to get a replacement, then if it gets a replacement, deletes the current value after replacing uses with the new value.<br class=""><br class="">Cheers,<br class="">Pete<br class=""><br class=""><0003.patch><br class=""></blockquote><br class="">One comment below:<br class=""><br class=""><blockquote type="cite" class="">commit 22105cea3de0994ad53c7868dbefa0122a3a3a44<br class="">Author: Peter Cooper <<a href="mailto:peter_cooper@apple.com" class="">peter_cooper@apple.com</a>><br class="">Date: Tue Jun 23 17:19:30 2015 -0700<br class=""><br class=""> Devirtualize replaceUsesOfWithOnConstant<br class=""><br class="">diff --git a/include/llvm/IR/GlobalValue.h b/include/llvm/IR/GlobalValue.h<br class="">index 4bca80e..c2a9d7e 100644<br class="">--- a/include/llvm/IR/GlobalValue.h<br class="">+++ b/include/llvm/IR/GlobalValue.h<br class="">@@ -92,7 +92,6 @@ private:<br class=""><br class=""> friend class Constant;<br class=""> void destroyConstantImpl();<br class="">- void replaceUsesOfWithOnConstant(Value *From, Value *To, Use *U) override;<br class=""></blockquote><br class="">Why do it differently for `GlobalValue`? Everywhere else, you define<br class="">the `Impl` method in the class, and call `llvm_unreachable()` within it.<br class=""></blockquote><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">No particular reason. I did GlobalValue first and was able to handle it with the macro, but for consistency I agree its better to make it a method.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class="">If there's a good reason for doing it differently (e.g., simplifying a<br class="">follow-up patch?), please document that in the commit message.<br class="">Otherwise being consistent seems simpler.<br class=""><br class="">LGTM, whatever you decide.<br class=""></blockquote><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Thanks! I’ll commit shortly with these small changes.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Pete</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""><blockquote type="cite" class=""><br class="">protected:<br class=""> /// \brief The intrinsic ID for this subclass (which must be a Function).<br class="">diff --git a/lib/IR/Constants.cpp b/lib/IR/Constants.cpp<br class="">index 9b2bb27..feb1628 100644<br class="">--- a/lib/IR/Constants.cpp<br class="">+++ b/lib/IR/Constants.cpp<br class="">@@ -2835,7 +2838,27 @@ Constant *ConstantDataVector::getSplatValue() const {<br class="">/// work, but would be really slow because it would have to unique each updated<br class="">/// array instance.<br class="">///<br class="">-void Constant::replaceUsesOfWithOnConstantImpl(Constant *Replacement) {<br class="">+void Constant::replaceUsesOfWithOnConstant(Value *From, Value *To, Use *U) {<br class="">+ Value *Replacement = nullptr;<br class="">+ switch (getValueID()) {<br class="">+ default:<br class="">+ llvm_unreachable("Not a constant!");<br class="">+#define HANDLE_GLOBAL_VALUE(Name) \<br class="">+ case Value::Name##Val: \<br class="">+ llvm_unreachable("You can't GV->replaceUsesOfWithOnConstant()!");<br class=""></blockquote><br class="">^ Here's the other difference.<br class=""><br class=""><blockquote type="cite" class="">+#define HANDLE_CONSTANT(Name) \<br class="">+ case Value::Name##Val: \<br class="">+ Replacement = \<br class="">+ cast<Name>(this)->replaceUsesOfWithOnConstantImpl(From, To, U); \<br class="">+ break;<br class="">+#include "llvm/IR/Value.def"<br class="">+ }<br class="">+<br class="">+ // If replaceUsesOfWithOnConstantImpl returned nullptr, then it handled<br class="">+ // replacing itself and we don't want to delete or replace anything else here.<br class="">+ if (!Replacement)<br class="">+ return;<br class="">+<br class=""> // I do need to replace this with an existing value.<br class=""> assert(Replacement != this && "I didn't contain From!");<br class=""><br class="">diff --git a/lib/IR/Globals.cpp b/lib/IR/Globals.cpp<br class="">index 49ac236..d84b671 100644<br class="">--- a/lib/IR/Globals.cpp<br class="">+++ b/lib/IR/Globals.cpp<br class="">@@ -48,10 +48,6 @@ void GlobalValue::destroyConstantImpl() {<br class=""> llvm_unreachable("You can't GV->destroyConstantImpl()!");<br class="">}<br class=""><br class="">-void GlobalValue::replaceUsesOfWithOnConstant(Value *From, Value *To, Use *U) {<br class="">- llvm_unreachable("You can't GV->replaceUsesOfWithOnConstant()!");<br class="">-}<br class="">-<br class=""></blockquote><br class="">^ And here. Seems like you could just rename this.<br class=""><br class=""><blockquote type="cite" class="">/// copyAttributesFrom - copy all additional attributes (those not needed to<br class="">/// create a GlobalValue) from the GlobalValue Src to this one.<br class="">void GlobalValue::copyAttributesFrom(const GlobalValue *Src) {<br class=""><br class=""></blockquote><br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">_______________________________________________</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">llvm-commits mailing list</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="mailto:llvm-commits@cs.uiuc.edu" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">llvm-commits@cs.uiuc.edu</a><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></div></blockquote></div><br class=""></div></div></body></html>