[llvm] r220645 - Revert "[PBQP] Unique-ptrify some PBQP Metadata structures. No functional change." (r220642)
Lang Hames
lhames at gmail.com
Sun Oct 26 14:38:00 PDT 2014
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.
Yeah - I actually really like dynarray for this - it's a pity to hear that
it got shot down.
- Lang.
On Sun, Oct 26, 2014 at 2:33 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> 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/833d3c86/attachment.html>
More information about the llvm-commits
mailing list