[llvm] r220645 - Revert "[PBQP] Unique-ptrify some PBQP Metadata structures. No functional change." (r220642)
Lang Hames
lhames at gmail.com
Sun Oct 26 15:06:53 PDT 2014
Copy/move operations explicitly defined in r220653. Lets see if that helps.
- Lang.
On Sun, Oct 26, 2014 at 2:38 PM, Lang Hames <lhames at gmail.com> wrote:
> 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/5a5ca082/attachment.html>
More information about the llvm-commits
mailing list