Hi all, I've located a regression that causes my project to crash. It's in revision 67979, where PointerIntPair is changed from storing the integer in the upper bits instead of the lower bits. My project is an experimental JIT-compiler in Windows. So I was wondering if anyone had any clue why the new PointerIntPair implementation might fail. It doesn't seem very safe to me to store other data into a pointer to begin with, but surely the lower bits must be a safer location than the upper bits? The actual crash I'm seeing is preceded by an assert in X86InstrInfo::sizeOfImm: "Immediate size not set!". Tracing backward, I'm seeing a MachineInstr with opcode MOV32rm and NumOperands equal to 6 (which is 5 in earlier revisions). I'm not sure if this actually helps explain the bug but it's one of the weird things that happen with revision 67979 and subsequent revisions. All help appreciated. Nicolas -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090501/3875d3cb/attachment.html>
Hi Nicolas, On 1-May-09, at 6:32 AM, Nicolas Capens wrote:> I’ve located a regression that causes my project to crash. It’s in > revision 67979, where PointerIntPair is changed from storing the > integer in the upper bits instead of the lower bits. My project is > an experimental JIT-compiler in Windows.We're looking into a similar bug right now. We see the problem elsewhere (early on in optimization, during the first instcombine pass) but it sounds like the same issue. We'll let you know when we know more.> So I was wondering if anyone had any clue why the new PointerIntPair > implementation might fail. It doesn’t seem very safe to me to store > other data into a pointer to begin with, but surely the lower bits > must be a safer location than the upper bits?Just to be clear, PointerIntPair doesn't (AFAIK) store the integer in the "high bits" of the pointer. It just uses the "higher parts" of the "low available bits" of the pointer. So, if a pointer is always aligned such that it always has 4 bits clear on the low side, and you only need to store one bit, it'll be stored in bit 3 (counting from the LSB towards the MSB). It may be that the alignment assumptions being made are simply wrong on Windows. So far we've been trying to track down whether this is actually the cause or if something's wrong with the Module coming into InstCombine, but given that you're seeing a similar problem elsewhere, it's not unlikely that this is the real cause.> The actual crash I’m seeing is preceded by an assert in > X86InstrInfo::sizeOfImm: "Immediate size not set!". Tracing > backward, I’m seeing a MachineInstr with opcode MOV32rm and > NumOperands equal to 6 (which is 5 in earlier revisions). I’m not > sure if this actually helps explain the bug but it’s one of the > weird things that happen with revision 67979 and subsequent revisions…-- Stefanus Du Toit <stefanus.dutoit at rapidmind.com> RapidMind Inc. phone: +1 519 885 5455 x116 -- fax: +1 519 885 1463 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090501/82133c08/attachment.html>
On 1-May-09, at 9:09 AM, Stefanus Du Toit wrote:> It may be that the alignment assumptions being made are simply wrong > on Windows.After quickly rereading the code for PointerIntPair this actually seems extremely unlikely, since there are nice asserts in the proper places. Stefanus -- Stefanus Du Toit <stefanus.dutoit at rapidmind.com> RapidMind Inc. phone: +1 519 885 5455 x116 -- fax: +1 519 885 1463
On May 1, 2009, at 6:09 AM, Stefanus Du Toit wrote:> Hi Nicolas, > > On 1-May-09, at 6:32 AM, Nicolas Capens wrote: >> I’ve located a regression that causes my project to crash. It’s in >> revision 67979, where PointerIntPair is changed from storing the >> integer in the upper bits instead of the lower bits. My project is >> an experimental JIT-compiler in Windows. > > We're looking into a similar bug right now. We see the problem > elsewhere (early on in optimization, during the first instcombine > pass) but it sounds like the same issue. We'll let you know when we > know more. > >> So I was wondering if anyone had any clue why the new >> PointerIntPair implementation might fail. It doesn’t seem very safe >> to me to store other data into a pointer to begin with, but surely >> the lower bits must be a safer location than the upper bits? > > Just to be clear, PointerIntPair doesn't (AFAIK) store the integer > in the "high bits" of the pointer. It just uses the "higher parts" > of the "low available bits" of the pointer. So, if a pointer is > always aligned such that it always has 4 bits clear on the low side, > and you only need to store one bit, it'll be stored in bit 3 > (counting from the LSB towards the MSB).FWIW, the reason it does this is to allow composable pointerintpairs, for example, PointerIntPair< PointerIntPair<foo*, 1>, 2> can use the low 3 bits as two different bitfields. -Chris -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090501/3af56906/attachment.html>
Hi Nicolas, Looks like Preston and I have found the cause of the problem. The issue is with PointerLikeTypeTraits<T*>::NumLowBitsAvailable. This is set to 3, which basically assumes that unless the traits are specialized for a particular pointer type, objects of that type are allocated with malloc() and aligned to 8 bytes. While PointerLikeTypeTraits is overloaded for Use*, it is not overloaded for User*. lib/VMCore/Use.cpp uses a PointerIntPair<User*, 1, Tag>, and things go bad. Users are typically allocated in an array after a bunch of Uses, which are 12 bytes in size, so they may actually only be aligned to 4 bytes. The attached patch (which I don't intend to commit, it's just a workaround) works around this simply by dropping this down to 2 bits, which gets us past our problem on Windows. I would appreciate if you could verify that this works for you as well. To actually solve this properly I see two main options: (1) we could specialize PointerLikeTypeTraits for User*, and leave the default value of NumLowBitsAvailable at 3. (2) we could drop the default NumLowBitsAvailable to 2 (or even use _alignof and similar compiler-specific extensions to determine it), and allow classes that assert that they are only ever allocated through malloc to relax it back up to 3. My preference would be for option (2), due to the difficulty of tracking this bug down, and the risk of future similar bugs happening. However, I'd appreciate some feedback from the LLVM developer community on this. Please let me know what you think, and I'll be happy to prepare a patch. I'm still wondering why this wasn't an issue on other platforms. One possibility is that Use is being bumped up to 16 bytes in size, and thus Users never get placed at less than an 8-byte boundary. However, in that case, the whole point of the "use diet" optimization is lost! I'll investigate and try to find out. I'm also still not sure why the assertion in PointerIntPair::setPointer() did not get triggered by the User::allocHungOffUses() implementation in Use.cpp. Perhaps the assertion is wrong (it looks reasonable, though) or perhaps there is something else going on I haven't seen yet. I'll look into this some more as well. Let me know if the workaround works for you, and I'd appreciate feedback from anyone (Chris? Gabor?) as to how best to proceed. Thanks! Stefanus On 1-May-09, at 6:32 AM, Nicolas Capens wrote:> Hi all, > > I’ve located a regression that causes my project to crash. It’s in > revision 67979, where PointerIntPair is changed from storing the > integer in the upper bits instead of the lower bits. My project is > an experimental JIT-compiler in Windows. > > So I was wondering if anyone had any clue why the new PointerIntPair > implementation might fail. It doesn’t seem very safe to me to store > other data into a pointer to begin with, but surely the lower bits > must be a safer location than the upper bits? > > The actual crash I’m seeing is preceded by an assert in > X86InstrInfo::sizeOfImm: "Immediate size not set!". Tracing > backward, I’m seeing a MachineInstr with opcode MOV32rm and > NumOperands equal to 6 (which is 5 in earlier revisions). I’m not > sure if this actually helps explain the bug but it’s one of the > weird things that happen with revision 67979 and subsequent revisions… > > All help appreciated. > > Nicolas > <ATT00001.txt>-- Stefanus Du Toit <stefanus.dutoit at rapidmind.com> RapidMind Inc. phone: +1 519 885 5455 x116 -- fax: +1 519 885 1463 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090501/a021811f/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: pointer_traits_workaround.diff Type: application/octet-stream Size: 979 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090501/a021811f/attachment.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090501/a021811f/attachment-0001.html>
On May 1, 2009, at 3:40 PM, Stefanus Du Toit wrote:> Hi Nicolas, > > Looks like Preston and I have found the cause of the problem. The > issue is with PointerLikeTypeTraits<T*>::NumLowBitsAvailable. This > is set to 3, which basically assumes that unless the traits are > specialized for a particular pointer type, objects of that type are > allocated with malloc() and aligned to 8 bytes.I still don't understand why this is a problem, but I decreased the default to 2 bits. Please verify that this helps, -Chris
On 2009-05-01, at 18:40, Stefanus Du Toit wrote:> Hi Nicolas, > > Looks like Preston and I have found the cause of the problem. The > issue is with PointerLikeTypeTraits<T*>::NumLowBitsAvailable. This > is set to 3, which basically assumes that unless the traits are > specialized for a particular pointer type, objects of that type are > allocated with malloc() and aligned to 8 bytes. > > While PointerLikeTypeTraits is overloaded for Use*, it is not > overloaded for User*. lib/VMCore/Use.cpp uses a > PointerIntPair<User*, 1, Tag>, and things go bad. Users are > typically allocated in an array after a bunch of Uses, which are 12 > bytes in size, so they may actually only be aligned to 4 bytes. > > The attached patch (which I don't intend to commit, it's just a > workaround) works around this simply by dropping this down to 2 > bits, which gets us past our problem on Windows. I would appreciate > if you could verify that this works for you as well. > > To actually solve this properly I see two main options: > > (1) we could specialize PointerLikeTypeTraits for User*, and leave > the default value of NumLowBitsAvailable at 3. > (2) we could drop the default NumLowBitsAvailable to 2 (or even use > _alignof and similar compiler-specific extensions to determine it), > and allow classes that assert that they are only ever allocated > through malloc to relax it back up to 3. >Something like this device should suffice; no need for extensions. template<typename T> class alignment_of { struct test { char a; T b; }; public: enum { value = sizeof(test) - sizeof(T) }; }; // usage: alignment_of<int>::value TR1's type_traits header is now available on many platforms now, too.> My preference would be for option (2), due to the difficulty of > tracking this bug down, and the risk of future similar bugs > happening. However, I'd appreciate some feedback from the LLVM > developer community on this. Please let me know what you think, and > I'll be happy to prepare a patch. > > I'm still wondering why this wasn't an issue on other platforms. One > possibility is that Use is being bumped up to 16 bytes in size, and > thus Users never get placed at less than an 8-byte boundary. > However, in that case, the whole point of the "use diet" > optimization is lost! I'll investigate and try to find out. > > I'm also still not sure why the assertion in > PointerIntPair::setPointer() did not get triggered by the > User::allocHungOffUses() implementation in Use.cpp. Perhaps the > assertion is wrong (it looks reasonable, though) or perhaps there is > something else going on I haven't seen yet. I'll look into this some > more as well. > > Let me know if the workaround works for you, and I'd appreciate > feedback from anyone (Chris? Gabor?) as to how best to proceed. > > Thanks! > > Stefanus > > > <pointer_traits_workaround.diff> > > On 1-May-09, at 6:32 AM, Nicolas Capens wrote: > >> Hi all, >> >> I’ve located a regression that causes my project to crash. It’s in >> revision 67979, where PointerIntPair is changed from storing the >> integer in the upper bits instead of the lower bits. My project is >> an experimental JIT-compiler in Windows. >> >> So I was wondering if anyone had any clue why the new >> PointerIntPair implementation might fail. It doesn’t seem very safe >> to me to store other data into a pointer to begin with, but surely >> the lower bits must be a safer location than the upper bits? >> >> The actual crash I’m seeing is preceded by an assert in >> X86InstrInfo::sizeOfImm: "Immediate size not set!". Tracing >> backward, I’m seeing a MachineInstr with opcode MOV32rm and >> NumOperands equal to 6 (which is 5 in earlier revisions). I’m not >> sure if this actually helps explain the bug but it’s one of the >> weird things that happen with revision 67979 and subsequent >> revisions… >> >> All help appreciated. >> >> Nicolas >> <ATT00001.txt> > > -- > Stefanus Du Toit <stefanus.dutoit at rapidmind.com> > RapidMind Inc. > phone: +1 519 885 5455 x116 -- fax: +1 519 885 1463 > > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090501/c0b03dee/attachment.html>
Hi Stefanus, I had actually tried that already, but my project kept crashing. However, after giving it a second try I noticed that the crash was happening elsewhere this time. After disabling the GVN optimization pass everything worked fine. Somewhere between revision 67979 and 67996 the bug that was causing GVN to fail was fixed. So thanks for tracking this down! I agree that option (2) is a good solution for now. But in my humble opinion the whole idea of storing other information in pointers is not very robust. One alternative would be to have a global store of all Value objects. They can then be identified with just an index instead of a pointer. This index can be stored in a bitfield that is smaller than 32-bit, leaving bits available for additional data. Cheers, Nicolas From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu] On Behalf Of Stefanus Du Toit Sent: zaterdag 2 mei 2009 0:41 To: LLVM Developers Mailing List Subject: Re: [LLVMdev] PointerIntPair causing trouble Hi Nicolas, Looks like Preston and I have found the cause of the problem. The issue is with PointerLikeTypeTraits<T*>::NumLowBitsAvailable. This is set to 3, which basically assumes that unless the traits are specialized for a particular pointer type, objects of that type are allocated with malloc() and aligned to 8 bytes. While PointerLikeTypeTraits is overloaded for Use*, it is not overloaded for User*. lib/VMCore/Use.cpp uses a PointerIntPair<User*, 1, Tag>, and things go bad. Users are typically allocated in an array after a bunch of Uses, which are 12 bytes in size, so they may actually only be aligned to 4 bytes. The attached patch (which I don't intend to commit, it's just a workaround) works around this simply by dropping this down to 2 bits, which gets us past our problem on Windows. I would appreciate if you could verify that this works for you as well. To actually solve this properly I see two main options: (1) we could specialize PointerLikeTypeTraits for User*, and leave the default value of NumLowBitsAvailable at 3. (2) we could drop the default NumLowBitsAvailable to 2 (or even use _alignof and similar compiler-specific extensions to determine it), and allow classes that assert that they are only ever allocated through malloc to relax it back up to 3. My preference would be for option (2), due to the difficulty of tracking this bug down, and the risk of future similar bugs happening. However, I'd appreciate some feedback from the LLVM developer community on this. Please let me know what you think, and I'll be happy to prepare a patch. I'm still wondering why this wasn't an issue on other platforms. One possibility is that Use is being bumped up to 16 bytes in size, and thus Users never get placed at less than an 8-byte boundary. However, in that case, the whole point of the "use diet" optimization is lost! I'll investigate and try to find out. I'm also still not sure why the assertion in PointerIntPair::setPointer() did not get triggered by the User::allocHungOffUses() implementation in Use.cpp. Perhaps the assertion is wrong (it looks reasonable, though) or perhaps there is something else going on I haven't seen yet. I'll look into this some more as well. Let me know if the workaround works for you, and I'd appreciate feedback from anyone (Chris? Gabor?) as to how best to proceed. Thanks! Stefanus -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090502/5ae048ed/attachment.html>