<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 19 February 2014 18:56, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div>On Wed, Feb 19, 2014 at 5:22 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>On 19 February 2014 16:25, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div>On Wed, Feb 19, 2014 at 3:46 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>On 18 February 2014 21:51, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">The 3 create functions look like they could be refactored into a template, perhaps? (even just as an implementation detail inside the .cpp file) even clone_impl could use such a thing - but clone_impl might be better implemented via a small CRTP intermediate template class (that way you don't have to do virtual dispatch as well as switching)?<br>
</div></blockquote><div><br></div></div><div>If we remove the vtable on value there would be 3 or so implementations all of which use switches internally (Value, Instruction and IntrinsicInst, maybe another in Constant somewhere?).</div>
</div></div></div></blockquote><div><br></div></div><div>Not sure which things you're referring to in that comment, but the 3 I see initially are IntrinsicInst::Create(..., Instruction*), IntrinsicInst::Create(..., BasicBlock*), and the 3rd isn't /quite/ the same, since it takes a 4th argument, an ArrayRef - so without something more general like perfect forwarding/variadic templates, it wouldn't generalize.<br>
<br>Generalizing the first two with a template might be nice, but since it doesn't generalize over all 3 I can see how that's not so interesting/useful.</div></div></div></div></blockquote><div><br></div></div><div>
Sorry, I was speaking of clone_impl(), not Create(). I was thinking that calling clone_impl on Value would handle Argument and a few others directly by looking at the ValueID, but ValueID is insufficiently specific to decide how to clone an Instruction, so it would dispatch to Instruction::clone_impl which would look at the Opcode of the Instruction, etc. There's another level for intrinsics and intrinsic IDs. Looking harder, I realize that clone() applies only to the Instruction hierarchy, not all of Value.</div>
<div><br></div><div>The four Create methods take Function*, optional ArrayRef<Value*> Args, NameStr, either BasicBlock *InsertAtEnd or Instruction *InsertBefore. A common way we generalize is to have one overly general form and have everything else call them, and I think we could forward to a single variant that has the Args and pass in an empty ArrayRef, but InsertAtEnd and InsertBefore aren't easily combined (is BasicBlock::end() a valid Instruction*?).</div>
</div></div></div></blockquote><div><br></div></div><div>Hmm, yeah, that's curious. Other containers use the "insert before this iterator" idiom as the single representation of specified insertion, including using the "insert before the end iterator" to insert at the end. I'm not sure if there's something that makes that not possible for BB - well, I guess the main thing is you're not dealing with iterators, you're dealing with Instruction *. If you can convert a valid Instruction* to an iterator trivially, then the underlying operation would take iterators and the one taking a BB would just pass the end iterator from that and the one taking the Instruction* would build an iterator from that and pass it.<br>
</div></div></div></div></blockquote><div><br></div><div>Right, Instruction * and BasicBlock::iterator are the same thing -- except that BB.end() returns a BasicBlock::iterator that isn't a valid Instruction. Whoops.</div>
<div><br></div><div>None of the intrinsics that have wrappers derived from IntrinsicInst take zero arguments, so I've eliminated those two constructors. I also noticed that two of CallInst's ctors were dead and eliminated them, and the copies of them in all the derived IntrinsicInsts. This means that IntrinsicInst now has 4 constructors, and all the derived wrappers have just 2. This is starting to look clean.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>And, yes, using the zero-length ArrayRef for zero args generalizes over that extra argument too.</div>
<div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">
<div class="gmail_extra"><div class="gmail_quote"><div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">The deleted special members in IntrinsicInst still look relevant - was there a reason they needed to be removed?<br>
</div></blockquote><div><br></div></div><div>Instinct to follow the rule of five, since IntrinsicInst now has an implicitly defined default constructor that does the right thing. I tried adding the deleted members back and everything still works correctly, so I assume I should do that.</div>
</div></div></div></blockquote><div><br></div></div><div>In general everything would "work" because there can't be any callers of these functions in the current codebase - the concern is if callers would appear now that those functions can be called and whether they would be correct. Slicing seems like a reasonable concern and I'd be inclined to delete the special members at the Instruction level, if not all the way up at Value.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Having to repeat the same set of ctors in every CallInst is a bit boring - could just template the 4th parameter and forward up to the base? Eventually these could/should be replaced with inheriting ctors, I assume?<br>
</div></blockquote><div><br></div></div><div>I'm not sure. [class.inhctor]/4 "A constructor so declared has the same access as the corresponding constructor in X". If this means that I can't go from protected to public then that's a problem.</div>
</div></div></div></blockquote><div><br></div></div><div>Hmm, fair point (alternatively it's probably OK to keep them protected and just have the derived classes friend IntrinsicInst since the ::Create functions are the only place that's meant to be building them I assume?)</div>
</div></div></div></blockquote><div><br></div></div><div>Right. Rereading, I need them to go from protected to private, which is safe to leave as protected. I think inheriting constructors will work in the C++11 future!</div>
</div></div></div></blockquote><div><br></div></div><div>OK - haven't fully wrapped my head around what you've got in mind here, but not sure it's necessary for me to do so in this code review (perhaps we can just chat about it offline). If they're private in the leaf classes, how will the base/intermediate class ::Create functions call them?</div>
</div></div></div></blockquote><div><br></div><div>They all friend IntrinsicInst.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote">
<div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>We can't use CRTP because we don't want to have multiple bases. I'm hopeful that there's some way to compact this, but keep in mind that we tend to favour obvious code and we're still using C++98.<br>
</div></div></div></div></blockquote><div><br></div></div><div>I wasn't suggesting having multiple bases - something like this:<br><br>class IntrinsicInst : ... {<br> ...<br>};<br><br><br>// template<tyepname T> // and when I got to here I realized what you meant about multiple bases because you have not just one level of derivation, but things like MemIntrinsic that are intermediate<br>
</div></div></div></div></blockquote><div><br></div></div><div>IntrinsicInst already derives from CallInst. I'd rather it didn't derive from both CallInst and also something else -- though I think it would work in this case, we generally try to avoid it in LLVM.</div>
<div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>// OK, so how about this:<br><br>
template<typename Base, typename Derived><br>
class ValueCloneHelper : public Base {<br>protected:<br> Derived *clone_impl() const LLVM_OVERRIDE {</div><div> return new Derived(static_cast<const Derived&>(*this));</div>
<div> }<br>};<br><br>class MemIntrinsic : public ValueCloneHelper<IntrinsicInst, MemIntrinsic> {</div><div>};<br><br>class MemSetInst : public ValueCloneHelper<MemIntrinsic, MemSetInst> {</div><div>};<br><br>
, etc... <br><br>Now, granted, those derived types either have to publicly copy constructible, which could risk other clients slicing, or they'd have to friend the ValueCloneHelper base.<br></div></div></div></div></blockquote>
<div><br></div></div><div>Various instantiations of ValueCloneHelper become part of the llvm::Value hierarchy? Sorry, I don't think this is an "LLVMy" design (I'm arguing elsewhere that mix-ins are the way to go for the Value hierarchy, but we aren't there yet).</div>
</div></div></div></blockquote><div><br></div></div><div>Curious - it's essentially /the/ canonical use of CRTP (which we do employ elsewhere) and seems ideal here. Admittedly the need for intermediate types in the hierarchy to be cloneable does add a bit of a wrinkle - on the flipside, in writing this out I discovered something I'd never actually written before: a generic CRTP device. It can be reused in any hierarchy that needs cloning, as long as the clone function is spelled the same way.</div>
</div></div></div></blockquote><div><br></div><div>I agree this is the way that CRTP is normally used. I don't mind CRTP in the implementation, and there are pieces designed especially for it (InstVisitor comes to mind), but this is going to exposed in the interface of LLVM. We have a high bar for keeping this interface a "beginner's" subset of C++, though exactly what that subset is has been in flux (CRTP is a well known device these days).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>If you want me to change it, I could break up the switch statement and give each class its own one-line implementation like the rest of the implementations...</div>
</div></div></div></blockquote></div><div><br>I'm not incredibly fussed by the difference if it's going to require repetition - though, yes, I'd tend to err on the side of just using virtual functions rather than virtual to a switch.<br>
</div></div></div></div></blockquote><div><br></div><div>I have a strong opinion that using virtual is wrong here and don't want to contribute to it, but you're right that makes my code the odd one out in its clone_impl implementation. Changed.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>...but when I delete the vtable from Value, I'm going to replace all the individual clone_impl functions with fewer functions that use switches. I can defer it until then.<br>
</div><div><br></div><div>
(The plan so far is to expose a public destructor on leaves only (concrete non-leaves will not expose their destructors since I don't want them to redo dispatch partway down the chain of destruction), and add a public destroy() method that will do manual dispatch to the most derived type's destructor. Subject to change as I actually implement it.)</div>
</div></div></div></blockquote><div><br></div></div><div>Rightio - guess I'll see it when I see it :)</div><div><br></div><div>- Dave</div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr">I assume the virtua dtors are to create anchors/key functions? Maybe a comment explaining that (or maybe not? it is pretty common in the codebase)<br>
</div></blockquote><div><br></div></div><div>They are. Comments added to the places that didn't have them and the destructor was empty.</div></div></div></div></blockquote></div><div><br>Sounds good.<br><br>- Dave</div>
</div>
</div></div>
</blockquote></div></div><br></div></div>
</blockquote></div></div><br></div></div>
</blockquote></div><br></div></div>