<div dir="ltr">Yep. The old code was deliberately written to use move constructors - I *should* have explicitly deleted the copy constructor before, then I might have seen this earlier. My guess is that PBQP would have crashed on Windows for the reason you indicated.<div><br></div><div>Yeah - I actually really like dynarray for this - it's a pity to hear that it got shot down.</div><div><br></div><div>- Lang.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Oct 26, 2014 at 2:33 PM, 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><div class="gmail_quote"><span class="">On Sun, Oct 26, 2014 at 2:26 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@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">From what Dave said it sounds like MSVC just flat out doesn't do this. I believe the problem is just that it (NodeMetadata) is a member of a copyable class (Graph::NodeEntry).</div></blockquote><div><br></div></span><div>That's a bit more interesting - and scarier for the old code, I imagine - if it was using the implicit copy ctor you'd be getting double deletes (because the copy would be pointing to the same new[] chunk).<br><br>This is where something like std::dynarray would be nice (so we could pay length + pointer + data, instead of length + capacity + pointer + data that we pay for with std::vector), but that was shot down in standardization, I believe... Chandler seems to think that was the right choice, but I don't really get it myself (something about how you could always just destroy and placement new the object anyhow, so it's not really ungrowing - but it still seems like a good thing to have to me). Copyable, non-resizable, dynamically sized-on-construction sequence.<br><br>We could just write our own for LLVM.</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>That's ok - the other (easier) solution is to just provide a sensible copy constructor.</div><div><br></div><div>- Lang.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Oct 26, 2014 at 2:23 PM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I tried commenting out the copy constructor declaration, but doesn't<br>
help - then it just tries to synthesize it itself. I don't know why<br>
it's not going for the move constructor and the compiler doesn't<br>
provide any clue as to who is moving/copying the object.<br>
<br>
If it's being used in some STL container or algorithm, maybe msvc's<br>
library doesn't know that it should move the object instead of copy<br>
it? :/<br>
<div><div><br>
On Sun, Oct 26, 2014 at 2:20 PM, Hans Wennborg <<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>> wrote:<br>
> Looks like it's still trying to use the now private copy constructor:<br>
><br>
> [3/5] Building CXX object lib\CodeGen\...s\LLVMCodeGen.dir\RegAllocPBQP.cpp.obj<br>
> FAILED: "C:/Program Files (x86)/CMake 2.8/bin/cmcldeps.exe" CXX ..\lib\CodeGen\R<br>
> egAllocPBQP.cpp "lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/RegAllocPBQP.cpp.obj.d"<br>
> lib\CodeGen\CMakeFiles\LLVMCodeGen.dir\RegAllocPBQP.cpp.obj "Note: including fi<br>
> le: " "C:/Program Files (x86)/Microsoft Visual Studio 11.0/VC/bin/cl.exe" C:\PR<br>
> OGRA~2\MICROS~3.0\VC\bin\cl.exe /nologo /TP /DWIN32 /D_WINDOWS /W3 /MT /O2 /<br>
> Ob2 -Ilib\CodeGen -I..\lib\CodeGen -Iinclude -I..\include -UNDEBUG -wd4146 -w<br>
> d4180 -wd4244 -wd4258 -wd4267 -wd4291 -wd4345 -wd4351 -wd4355 -wd4503 -wd4624 -w<br>
> d4722 -wd4800 -w14062 -we4238 /EHs-c- /GR- -DGTEST_HAS_RTTI=0 -D_CRT_NONSTDC_NO<br>
> _DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_N<br>
> O_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNI<br>
> NGS -D_VARIADIC_MAX=10 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_<br>
> LIMIT_MACROS /Folib\CodeGen\CMakeFiles\LLVMCodeGen.dir\RegAllocPBQP.cpp.obj /Fdl<br>
> ib\CodeGen\CMakeFiles\LLVMCodeGen.dir/ -c ..\lib\CodeGen\RegAllocPBQP.cpp<br>
> d:\src\llvm\include\llvm\codegen\pbqp\Graph.h(100) : error C2248: 'llvm::PBQP::R<br>
> egAlloc::NodeMetadata::NodeMetadata' : cannot access private member declared in<br>
> class 'llvm::PBQP::RegAlloc::NodeMetadata'<br>
> ..\include\llvm/CodeGen/RegAllocPBQP.h(141) : see declaration of 'llvm::<br>
> PBQP::RegAlloc::NodeMetadata::NodeMetadata'<br>
> ..\include\llvm/CodeGen/RegAllocPBQP.h(76) : see declaration of 'llvm::P<br>
> BQP::RegAlloc::NodeMetadata'<span><br>
> This diagnostic occurred in the compiler generated function 'llvm::PBQP:<br></span>
> :Graph<SolverT>::NodeEntry::NodeEntry(const llvm::PBQP::Graph<SolverT>::NodeEntr<br>
> y &)'<br>
> with<br>
> [<br>
> SolverT=llvm::PBQP::RegAlloc::RegAllocSolverImpl<br>
> ]<br>
> ninja: build stopped: subcommand failed.<br>
><br>
><br>
> On Sun, Oct 26, 2014 at 2:09 PM, Lang Hames <<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>> wrote:<br>
>> Hi Hans,<br>
>><br>
>> Thanks for the insight. I've tried to work around this by explicitly<br>
>> defining a move constructor (and deleting the copy constructor) in r220649.<br>
>> Hopefully that works.<br>
>><br>
>> Cheers,<br>
>> Lang.<br>
>><span><br>
>> On Sun, Oct 26, 2014 at 1:47 PM, Hans Wennborg <<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>> wrote:<br>
>>><br></span><div><div>
>>> On Sun, Oct 26, 2014 at 1:44 PM, Lang Hames <<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>> wrote:<br>
>>> > Oh:<br>
>>> ><br>
>>> > This diagnostic occurred in the compiler generated function<br>
>>> > 'llvm::PBQP::RegAlloc::NodeMetadata::NodeMetadata(const<br>
>>> > llvm::PBQP::RegAlloc::NodeMetadata &)'<br>
>>> ><br>
>>> > That explains that, though it's less clear why VC thinks a copy<br>
>>> > constructor<br>
>>> > is needed.<br>
>>> ><br>
>>> > Will have a fix shortly.<br>
>>><br>
>>> Thanks! I think we've run into similar problems before where some<br>
>>> object gets moved but VC is synthesizing a copy constructor instead,<br>
>>> and fails to copy the unique_ptr member.<br>
>>><br>
>>><br>
>>> > On Sun, Oct 26, 2014 at 1:36 PM, Lang Hames <<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>> wrote:<br>
>>> >><br>
>>> >> That's odd. It's just a unique_ptr to an array - that should be<br>
>>> >> supported.<br>
>>> >> I'm going to try #including memory directly and recommit.<br>
>>> >><br>
>>> >> - Lang.<br>
>>> >><br>
>>> >> On Sun, Oct 26, 2014 at 12:50 PM, Hans Wennborg <<a href="mailto:hans@hanshq.net" target="_blank">hans@hanshq.net</a>><br>
>>> >> wrote:<br>
>>> >>><br>
>>> >>> Author: hans<br>
>>> >>> Date: Sun Oct 26 14:50:13 2014<br>
>>> >>> New Revision: 220645<br>
>>> >>><br>
>>> >>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=220645&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=220645&view=rev</a><br>
>>> >>> Log:<br>
>>> >>> Revert "[PBQP] Unique-ptrify some PBQP Metadata structures. No<br>
>>> >>> functional<br>
>>> >>> change." (r220642)<br>
>>> >>><br>
>>> >>> It broke the Windows build:<br>
>>> >>><br>
>>> >>> [1/19] Building CXX object<br>
>>> >>> lib\CodeGen\CMakeFiles\LLVMCodeGen.dir\RegAllocPBQP.cpp.obj<br>
>>> >>><br>
>>> >>><br>
>>> >>> C:\bb-win7\ninja-clang-i686-msc17-R\llvm-project\llvm\include\llvm/CodeGen/RegAllocPBQP.h(132)<br>
>>> >>> : error C2248: 'std::unique_ptr<_Ty>::unique_ptr' : cannot access<br>
>>> >>> private<br>
>>> >>> member declared in class 'std::unique_ptr<_Ty>'<br>
>>> >>><br>
>>> >>> with<br>
>>> >>> [<br>
>>> >>> _Ty=unsigned int []<br>
>>> >>> ]<br>
>>> >>> D:\Program Files (x86)\Microsoft Visual Studio<br>
>>> >>> 11.0\VC\include\memory(1600) : see declaration of<br>
>>> >>> 'std::unique_ptr<_Ty>::unique_ptr'<br>
>>> >>> with<br>
>>> >>> [<br>
>>> >>> _Ty=unsigned int []<br>
>>> >>> ]<br>
>>> >>> This diagnostic occurred in the compiler generated function<br>
>>> >>> 'llvm::PBQP::RegAlloc::NodeMetadata::NodeMetadata(const<br>
>>> >>> llvm::PBQP::RegAlloc::NodeMetadata &)'<br>
>>> >>><br>
>>> >>> Modified:<br>
>>> >>> llvm/trunk/include/llvm/CodeGen/RegAllocPBQP.h<br>
>>> >>><br>
>>> >>> Modified: llvm/trunk/include/llvm/CodeGen/RegAllocPBQP.h<br>
>>> >>> URL:<br>
>>> >>><br>
>>> >>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/RegAllocPBQP.h?rev=220645&r1=220644&r2=220645&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/RegAllocPBQP.h?rev=220645&r1=220644&r2=220645&view=diff</a><br>
>>> >>><br>
>>> >>><br>
>>> >>> ==============================================================================<br>
>>> >>> --- llvm/trunk/include/llvm/CodeGen/RegAllocPBQP.h (original)<br>
>>> >>> +++ llvm/trunk/include/llvm/CodeGen/RegAllocPBQP.h Sun Oct 26 14:50:13<br>
>>> >>> 2014<br>
>>> >>> @@ -62,15 +62,20 @@ public:<br>
>>> >>> delete[] ColCounts;<br>
>>> >>> }<br>
>>> >>><br>
>>> >>> + ~MatrixMetadata() {<br>
>>> >>> + delete[] UnsafeRows;<br>
>>> >>> + delete[] UnsafeCols;<br>
>>> >>> + }<br>
>>> >>> +<br>
>>> >>> unsigned getWorstRow() const { return WorstRow; }<br>
>>> >>> unsigned getWorstCol() const { return WorstCol; }<br>
>>> >>> - const bool* getUnsafeRows() const { return UnsafeRows.get(); }<br>
>>> >>> - const bool* getUnsafeCols() const { return UnsafeCols.get(); }<br>
>>> >>> + const bool* getUnsafeRows() const { return UnsafeRows; }<br>
>>> >>> + const bool* getUnsafeCols() const { return UnsafeCols; }<br>
>>> >>><br>
>>> >>> private:<br>
>>> >>> unsigned WorstRow, WorstCol;<br>
>>> >>> - std::unique_ptr<bool[]> UnsafeRows;<br>
>>> >>> - std::unique_ptr<bool[]> UnsafeCols;<br>
>>> >>> + bool* UnsafeRows;<br>
>>> >>> + bool* UnsafeCols;<br>
>>> >>> };<br>
>>> >>><br>
>>> >>> class NodeMetadata {<br>
>>> >>> @@ -83,6 +88,7 @@ public:<br>
>>> >>> NotProvablyAllocatable } ReductionState;<br>
>>> >>><br>
>>> >>> NodeMetadata() : RS(Unprocessed), DeniedOpts(0),<br>
>>> >>> OptUnsafeEdges(nullptr){}<br>
>>> >>> + ~NodeMetadata() { delete[] OptUnsafeEdges; }<br>
>>> >>><br>
>>> >>> void setVReg(unsigned VReg) { this->VReg = VReg; }<br>
>>> >>> unsigned getVReg() const { return VReg; }<br>
>>> >>> @@ -94,7 +100,7 @@ public:<br>
>>> >>><br>
>>> >>> void setup(const Vector& Costs) {<br>
>>> >>> NumOpts = Costs.getLength() - 1;<br>
>>> >>> - OptUnsafeEdges = std::unique_ptr<unsigned[]>(new<br>
>>> >>> unsigned[NumOpts]());<br>
>>> >>> + OptUnsafeEdges = new unsigned[NumOpts]();<br>
>>> >>> }<br>
>>> >>><br>
>>> >>> ReductionState getReductionState() const { return RS; }<br>
>>> >>> @@ -118,15 +124,15 @@ public:<br>
>>> >>><br>
>>> >>> bool isConservativelyAllocatable() const {<br>
>>> >>> return (DeniedOpts < NumOpts) ||<br>
>>> >>> - (std::find(&OptUnsafeEdges[0], &OptUnsafeEdges[NumOpts], 0) !=<br>
>>> >>> - &OptUnsafeEdges[NumOpts]);<br>
>>> >>> + (std::find(OptUnsafeEdges, OptUnsafeEdges + NumOpts, 0) !=<br>
>>> >>> + OptUnsafeEdges + NumOpts);<br>
>>> >>> }<br>
>>> >>><br>
>>> >>> private:<br>
>>> >>> ReductionState RS;<br>
>>> >>> unsigned NumOpts;<br>
>>> >>> unsigned DeniedOpts;<br>
>>> >>> - std::unique_ptr<unsigned[]> OptUnsafeEdges;<br>
>>> >>> + unsigned* OptUnsafeEdges;<br>
>>> >>> unsigned VReg;<br>
>>> >>> OptionToRegMap OptionRegs;<br>
>>> >>> };<br>
>>> >>><br>
>>> >>><br>
>>> >>> _______________________________________________<br>
>>> >>> llvm-commits mailing list<br>
>>> >>> <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>>> >>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>>> >><br>
>>> >><br>
>>> ><br>
>>> ><br>
>>> > _______________________________________________<br>
>>> > llvm-commits mailing list<br>
>>> > <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>>> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>>> ><br>
>><br>
>><br>
</div></div></div></div></blockquote></div><br></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>