[llvm] r220645 - Revert "[PBQP] Unique-ptrify some PBQP Metadata structures. No functional change." (r220642)
David Blaikie
dblaikie at gmail.com
Sun Oct 26 14:33:44 PDT 2014
On Sun, Oct 26, 2014 at 2:26 PM, Lang Hames <lhames at gmail.com> wrote:
> 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).
>
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).
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.
We could just write our own for LLVM.
>
> That's ok - the other (easier) solution is to just provide a sensible copy
> constructor.
>
> - Lang.
>
> On Sun, Oct 26, 2014 at 2:23 PM, Hans Wennborg <hans at chromium.org> wrote:
>
>> I tried commenting out the copy constructor declaration, but doesn't
>> help - then it just tries to synthesize it itself. I don't know why
>> it's not going for the move constructor and the compiler doesn't
>> provide any clue as to who is moving/copying the object.
>>
>> If it's being used in some STL container or algorithm, maybe msvc's
>> library doesn't know that it should move the object instead of copy
>> it? :/
>>
>> On Sun, Oct 26, 2014 at 2:20 PM, Hans Wennborg <hans at chromium.org> wrote:
>> > Looks like it's still trying to use the now private copy constructor:
>> >
>> > [3/5] Building CXX object
>> lib\CodeGen\...s\LLVMCodeGen.dir\RegAllocPBQP.cpp.obj
>> > FAILED: "C:/Program Files (x86)/CMake 2.8/bin/cmcldeps.exe" CXX
>> ..\lib\CodeGen\R
>> > egAllocPBQP.cpp
>> "lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/RegAllocPBQP.cpp.obj.d"
>> > lib\CodeGen\CMakeFiles\LLVMCodeGen.dir\RegAllocPBQP.cpp.obj "Note:
>> including fi
>> > le: " "C:/Program Files (x86)/Microsoft Visual Studio
>> 11.0/VC/bin/cl.exe" C:\PR
>> > OGRA~2\MICROS~3.0\VC\bin\cl.exe /nologo /TP /DWIN32 /D_WINDOWS /W3
>> /MT /O2 /
>> > Ob2 -Ilib\CodeGen -I..\lib\CodeGen -Iinclude -I..\include -UNDEBUG
>> -wd4146 -w
>> > d4180 -wd4244 -wd4258 -wd4267 -wd4291 -wd4345 -wd4351 -wd4355 -wd4503
>> -wd4624 -w
>> > d4722 -wd4800 -w14062 -we4238 /EHs-c- /GR- -DGTEST_HAS_RTTI=0
>> -D_CRT_NONSTDC_NO
>> > _DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE
>> -D_CRT_SECURE_N
>> > O_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE
>> -D_SCL_SECURE_NO_WARNI
>> > NGS -D_VARIADIC_MAX=10 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS
>> -D__STDC_
>> > LIMIT_MACROS
>> /Folib\CodeGen\CMakeFiles\LLVMCodeGen.dir\RegAllocPBQP.cpp.obj /Fdl
>> > ib\CodeGen\CMakeFiles\LLVMCodeGen.dir/ -c
>> ..\lib\CodeGen\RegAllocPBQP.cpp
>> > d:\src\llvm\include\llvm\codegen\pbqp\Graph.h(100) : error C2248:
>> 'llvm::PBQP::R
>> > egAlloc::NodeMetadata::NodeMetadata' : cannot access private member
>> declared in
>> > class 'llvm::PBQP::RegAlloc::NodeMetadata'
>> > ..\include\llvm/CodeGen/RegAllocPBQP.h(141) : see declaration
>> of 'llvm::
>> > PBQP::RegAlloc::NodeMetadata::NodeMetadata'
>> > ..\include\llvm/CodeGen/RegAllocPBQP.h(76) : see declaration of
>> 'llvm::P
>> > BQP::RegAlloc::NodeMetadata'
>> > This diagnostic occurred in the compiler generated function
>> 'llvm::PBQP:
>> > :Graph<SolverT>::NodeEntry::NodeEntry(const
>> llvm::PBQP::Graph<SolverT>::NodeEntr
>> > y &)'
>> > with
>> > [
>> > SolverT=llvm::PBQP::RegAlloc::RegAllocSolverImpl
>> > ]
>> > ninja: build stopped: subcommand failed.
>> >
>> >
>> > On Sun, Oct 26, 2014 at 2:09 PM, Lang Hames <lhames at gmail.com> wrote:
>> >> Hi Hans,
>> >>
>> >> Thanks for the insight. I've tried to work around this by explicitly
>> >> defining a move constructor (and deleting the copy constructor) in
>> r220649.
>> >> Hopefully that works.
>> >>
>> >> Cheers,
>> >> Lang.
>> >>
>> >> On Sun, Oct 26, 2014 at 1:47 PM, Hans Wennborg <hans at chromium.org>
>> wrote:
>> >>>
>> >>> On Sun, Oct 26, 2014 at 1:44 PM, Lang Hames <lhames at gmail.com> wrote:
>> >>> > Oh:
>> >>> >
>> >>> > This diagnostic occurred in the compiler generated function
>> >>> > 'llvm::PBQP::RegAlloc::NodeMetadata::NodeMetadata(const
>> >>> > llvm::PBQP::RegAlloc::NodeMetadata &)'
>> >>> >
>> >>> > That explains that, though it's less clear why VC thinks a copy
>> >>> > constructor
>> >>> > is needed.
>> >>> >
>> >>> > Will have a fix shortly.
>> >>>
>> >>> Thanks! I think we've run into similar problems before where some
>> >>> object gets moved but VC is synthesizing a copy constructor instead,
>> >>> and fails to copy the unique_ptr member.
>> >>>
>> >>>
>> >>> > On Sun, Oct 26, 2014 at 1:36 PM, Lang Hames <lhames at gmail.com>
>> wrote:
>> >>> >>
>> >>> >> That's odd. It's just a unique_ptr to an array - that should be
>> >>> >> supported.
>> >>> >> I'm going to try #including memory directly and recommit.
>> >>> >>
>> >>> >> - Lang.
>> >>> >>
>> >>> >> On Sun, Oct 26, 2014 at 12:50 PM, Hans Wennborg <hans at hanshq.net>
>> >>> >> wrote:
>> >>> >>>
>> >>> >>> Author: hans
>> >>> >>> Date: Sun Oct 26 14:50:13 2014
>> >>> >>> New Revision: 220645
>> >>> >>>
>> >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=220645&view=rev
>> >>> >>> Log:
>> >>> >>> Revert "[PBQP] Unique-ptrify some PBQP Metadata structures. No
>> >>> >>> functional
>> >>> >>> change." (r220642)
>> >>> >>>
>> >>> >>> It broke the Windows build:
>> >>> >>>
>> >>> >>> [1/19] Building CXX object
>> >>> >>> lib\CodeGen\CMakeFiles\LLVMCodeGen.dir\RegAllocPBQP.cpp.obj
>> >>> >>>
>> >>> >>>
>> >>> >>>
>> C:\bb-win7\ninja-clang-i686-msc17-R\llvm-project\llvm\include\llvm/CodeGen/RegAllocPBQP.h(132)
>> >>> >>> : error C2248: 'std::unique_ptr<_Ty>::unique_ptr' : cannot access
>> >>> >>> private
>> >>> >>> member declared in class 'std::unique_ptr<_Ty>'
>> >>> >>>
>> >>> >>> with
>> >>> >>> [
>> >>> >>> _Ty=unsigned int []
>> >>> >>> ]
>> >>> >>> D:\Program Files (x86)\Microsoft Visual Studio
>> >>> >>> 11.0\VC\include\memory(1600) : see declaration of
>> >>> >>> 'std::unique_ptr<_Ty>::unique_ptr'
>> >>> >>> with
>> >>> >>> [
>> >>> >>> _Ty=unsigned int []
>> >>> >>> ]
>> >>> >>> This diagnostic occurred in the compiler generated function
>> >>> >>> 'llvm::PBQP::RegAlloc::NodeMetadata::NodeMetadata(const
>> >>> >>> llvm::PBQP::RegAlloc::NodeMetadata &)'
>> >>> >>>
>> >>> >>> Modified:
>> >>> >>> llvm/trunk/include/llvm/CodeGen/RegAllocPBQP.h
>> >>> >>>
>> >>> >>> Modified: llvm/trunk/include/llvm/CodeGen/RegAllocPBQP.h
>> >>> >>> URL:
>> >>> >>>
>> >>> >>>
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/RegAllocPBQP.h?rev=220645&r1=220644&r2=220645&view=diff
>> >>> >>>
>> >>> >>>
>> >>> >>>
>> ==============================================================================
>> >>> >>> --- llvm/trunk/include/llvm/CodeGen/RegAllocPBQP.h (original)
>> >>> >>> +++ llvm/trunk/include/llvm/CodeGen/RegAllocPBQP.h Sun Oct 26
>> 14:50:13
>> >>> >>> 2014
>> >>> >>> @@ -62,15 +62,20 @@ public:
>> >>> >>> delete[] ColCounts;
>> >>> >>> }
>> >>> >>>
>> >>> >>> + ~MatrixMetadata() {
>> >>> >>> + delete[] UnsafeRows;
>> >>> >>> + delete[] UnsafeCols;
>> >>> >>> + }
>> >>> >>> +
>> >>> >>> unsigned getWorstRow() const { return WorstRow; }
>> >>> >>> unsigned getWorstCol() const { return WorstCol; }
>> >>> >>> - const bool* getUnsafeRows() const { return UnsafeRows.get(); }
>> >>> >>> - const bool* getUnsafeCols() const { return UnsafeCols.get(); }
>> >>> >>> + const bool* getUnsafeRows() const { return UnsafeRows; }
>> >>> >>> + const bool* getUnsafeCols() const { return UnsafeCols; }
>> >>> >>>
>> >>> >>> private:
>> >>> >>> unsigned WorstRow, WorstCol;
>> >>> >>> - std::unique_ptr<bool[]> UnsafeRows;
>> >>> >>> - std::unique_ptr<bool[]> UnsafeCols;
>> >>> >>> + bool* UnsafeRows;
>> >>> >>> + bool* UnsafeCols;
>> >>> >>> };
>> >>> >>>
>> >>> >>> class NodeMetadata {
>> >>> >>> @@ -83,6 +88,7 @@ public:
>> >>> >>> NotProvablyAllocatable } ReductionState;
>> >>> >>>
>> >>> >>> NodeMetadata() : RS(Unprocessed), DeniedOpts(0),
>> >>> >>> OptUnsafeEdges(nullptr){}
>> >>> >>> + ~NodeMetadata() { delete[] OptUnsafeEdges; }
>> >>> >>>
>> >>> >>> void setVReg(unsigned VReg) { this->VReg = VReg; }
>> >>> >>> unsigned getVReg() const { return VReg; }
>> >>> >>> @@ -94,7 +100,7 @@ public:
>> >>> >>>
>> >>> >>> void setup(const Vector& Costs) {
>> >>> >>> NumOpts = Costs.getLength() - 1;
>> >>> >>> - OptUnsafeEdges = std::unique_ptr<unsigned[]>(new
>> >>> >>> unsigned[NumOpts]());
>> >>> >>> + OptUnsafeEdges = new unsigned[NumOpts]();
>> >>> >>> }
>> >>> >>>
>> >>> >>> ReductionState getReductionState() const { return RS; }
>> >>> >>> @@ -118,15 +124,15 @@ public:
>> >>> >>>
>> >>> >>> bool isConservativelyAllocatable() const {
>> >>> >>> return (DeniedOpts < NumOpts) ||
>> >>> >>> - (std::find(&OptUnsafeEdges[0], &OptUnsafeEdges[NumOpts],
>> 0) !=
>> >>> >>> - &OptUnsafeEdges[NumOpts]);
>> >>> >>> + (std::find(OptUnsafeEdges, OptUnsafeEdges + NumOpts, 0) !=
>> >>> >>> + OptUnsafeEdges + NumOpts);
>> >>> >>> }
>> >>> >>>
>> >>> >>> private:
>> >>> >>> ReductionState RS;
>> >>> >>> unsigned NumOpts;
>> >>> >>> unsigned DeniedOpts;
>> >>> >>> - std::unique_ptr<unsigned[]> OptUnsafeEdges;
>> >>> >>> + unsigned* OptUnsafeEdges;
>> >>> >>> unsigned VReg;
>> >>> >>> OptionToRegMap OptionRegs;
>> >>> >>> };
>> >>> >>>
>> >>> >>>
>> >>> >>> _______________________________________________
>> >>> >>> llvm-commits mailing list
>> >>> >>> llvm-commits at cs.uiuc.edu
>> >>> >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >>> >>
>> >>> >>
>> >>> >
>> >>> >
>> >>> > _______________________________________________
>> >>> > llvm-commits mailing list
>> >>> > llvm-commits at cs.uiuc.edu
>> >>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >>> >
>> >>
>> >>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141026/2a44304b/attachment.html>
More information about the llvm-commits
mailing list