<div dir="ltr"><div>Alright, here's another revised patch.</div><div><br></div><div>PL</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 1, 2017 at 2:51 AM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Puyan Lotfi <<a href="mailto:puyan.lotfi.llvm@gmail.com">puyan.lotfi.llvm@gmail.com</a>> writes:<br>
> On Tue, Oct 31, 2017 at 2:15 AM, Justin Bogner <<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a>> wrote:<br>
>> Puyan Lotfi <<a href="mailto:puyan.lotfi.llvm@gmail.com">puyan.lotfi.llvm@gmail.com</a>> writes:<br>
>> > Thanks Justin!<br>
>> ><br>
>> > I have an updated patch, addressing the provided feedback.<br>
>><br>
>> Looking better. I have a few more nitpicks below, and this could<br>
>> probably use a couple of simple tests for the basic functionality.<br>
</span> ...<br>
<span class="">>> > +  TypedVReg(const TypedVReg &TR): type(TR.type), reg(TR.reg) { }<br>
>> > +<br>
>> > +  TypedVReg& operator=(const TypedVReg &rhs) {<br>
>> > +    type = rhs.type;<br>
>> > +    reg = rhs.reg;<br>
>> > +    return *this;<br>
>> > +  }<br>
>><br>
>> These should both be default, so just omit them (or write them as =<br>
>> default if you're feeling verbose).<br>
><br>
> I can be sure the default = operator is going to copy the members from rhs<br>
> to this?<br>
<br>
</span>Yes, the default operator= does memberwise copy (or memmove-like<br>
semantics when the operator is trivial, like in this case). Leaving it<br>
as the default also means you'll get move assignment and such for free.<br>
<br>
There's a good summary of the standard here:<br>
<br>
  <a href="http://en.cppreference.com/w/cpp/language/copy_assignment" rel="noreferrer" target="_blank">http://en.cppreference.com/w/<wbr>cpp/language/copy_assignment</a><br>
<span class=""><br>
>> > +  const TargetRegisterClass *DummyRC = [] (const MachineFunction &MF) {<br>
>> > +    const unsigned DummyVReg = GetDummyVReg(MF);<br>
>> > +    assert(DummyVReg != ~0U && "Could not find a dummy VReg.");<br>
>><br>
>> So this pass doesn't work at all if we give it input with no vregs. That<br>
>> doesn't seem great - maybe we'd be better off making something up in<br>
>> that case.<br>
><br>
> Maybe I should bail out of the vreg renaming portion but continue the<br>
> instruction ordering?<br>
<br>
</span>That sounds reasonable to me as a first shot at this. We can make it<br>
more clever in follow ups.<br>
<div class="HOEnZb"><div class="h5"><br>
> I think I shouldn't assert but if I can't find a vreg then in the current<br>
> form of the pass im not sure exactly what I could rename in the first<br>
> place. Hmm..<br>
><br>
> I'm still working on another patch revision. Will reply with it soon.<br>
</div></div></blockquote></div><br></div>