[llvm] r278532 - ADT: Remove the ilist_nextprev_traits customization point
Yung, Douglas via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 12 16:12:02 PDT 2016
Hi Duncan,
Your change in r278572 seems to have helped, the number of failures is down to 8. (http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/9911). (The sancov failure is unrelated to your change) I'm still building a debug build of the compiler and will send a callstack for the failure when I have it.
Douglas Yung
PS - Sorry, I did miss the rest of the thread, I didn't see it because my email client had not indexed the emails yet so they did not appear when I was searching. Sorry about that!
> -----Original Message-----
> From: dexonsmith at apple.com [mailto:dexonsmith at apple.com]
> Sent: Friday, August 12, 2016 16:05
> To: Yung, Douglas
> Cc: Hans Wennborg; llvm-commits at lists.llvm.org
> Subject: Re: [llvm] r278532 - ADT: Remove the ilist_nextprev_traits
> customization point
>
> Can you give me the backtraces? I strongly suspect a similar problem,
> but I can't investigate on Darwin without knowing where it crashes.
> (Perhaps r278572 will be sufficient...)
>
> > On 2016-Aug-12, at 15:54, Yung, Douglas <douglas.yung at sony.com>
> wrote:
> >
> > Hi Duncan,
> >
> > I just wanted to add that I'm seeing 13 failures in the regression
> tests on the Windows PS4 bot (http://lab.llvm.org:8011/builders/llvm-
> clang-lld-x86_64-scei-ps4-windows10pro-fast) that I suspect are due to
> this change. The problem only seems to exist for us when building on
> Windows, so that might be why Hans has not been able to reproduce it on
> linux. To narrow it down, I synced up to r278532, saw the build
> failure, and then patched it with the subsequent fixes, r278537 and
> r278539 only. Once I did that, I saw the following 13 test failures:
> >
> > Failing Tests (13):
> > LLVM :: CodeGen/X86/2010-02-23-RematImplicitSubreg.ll
> > LLVM :: CodeGen/X86/2011-03-08-Sched-crash.ll
> > LLVM :: CodeGen/X86/2012-08-17-legalizer-crash.ll
> > LLVM :: CodeGen/X86/avx-splat.ll
> > LLVM :: CodeGen/X86/compare-inf.ll
> > LLVM :: CodeGen/X86/copy-eflags.ll
> > LLVM :: CodeGen/X86/fast-isel-i1.ll
> > LLVM :: CodeGen/X86/machine-cse.ll
> > LLVM :: CodeGen/X86/no-and8ri8.ll
> > LLVM :: CodeGen/X86/shrink-wrap-chkstk.ll
> > LLVM :: CodeGen/X86/x86-shrink-wrap-unwind.ll
> > LLVM :: CodeGen/X86/x86-shrink-wrapping.ll
> > LLVM :: Transforms/CodeExtractor/X86/InheritTargetAttributes.ll
> >
> > It's not immediately obvious to me why your change is causing these
> failures, but this (as well as another failure) is causing the PS4
> Windows bot to be red at the moment. Can you take a look into this?
> >
> > Douglas Yung
> >
> >> -----Original Message-----
> >> From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On
> >> Behalf Of Hans Wennborg via llvm-commits
> >> Sent: Friday, August 12, 2016 14:49
> >> To: Duncan P. N. Exon Smith
> >> Cc: llvm-commits
> >> Subject: Re: [llvm] r278532 - ADT: Remove the ilist_nextprev_traits
> >> customization point
> >>
> >> Just a heads up I'm getting compiler crashes building Chromium on
> >> Windows with this (well, r278539 really). Still not sure what's
> going
> >> on, though.. http://crbug.com/637413
> >>
> >> It's crashing here:
> >>
> >> void X86FrameLowering::emitEpilogue(MachineFunction &MF,
> >> MachineBasicBlock &MBB) const {
> >> const MachineFrameInfo &MFI = MF.getFrameInfo();
> >> X86MachineFunctionInfo *X86FI =
> MF.getInfo<X86MachineFunctionInfo>();
> >> MachineBasicBlock::iterator MBBI = MBB.getFirstTerminator();
> >> unsigned RetOpcode = MBBI->getOpcode(); <--- CRASH
> >>
> >>
> >> I assume MBBI is an ilist_iterator<> of some kind, but I still
> >> haven't been able to figure out what's up, or reproduce the crash on
> >> Linux :-/
> >>
> >>
> >> On Fri, Aug 12, 2016 at 10:32 AM, Duncan P. N. Exon Smith via llvm-
> >> commits <llvm-commits at lists.llvm.org> wrote:
> >>> Author: dexonsmith
> >>> Date: Fri Aug 12 12:32:34 2016
> >>> New Revision: 278532
> >>>
> >>> URL: http://llvm.org/viewvc/llvm-project?rev=278532&view=rev
> >>> Log:
> >>> ADT: Remove the ilist_nextprev_traits customization point
> >>>
> >>> No one is using the capability to implement next and prev another
> >>> way (since lld stopped doing it in r278468). Remove the
> >>> customization point by moving the API from ilist_nextprev_traits<T>
> >>> to
> >> ilist_node_access.
> >>>
> >>> The old traits class is still useful/necessary API as a target for
> >>> friends of node types that inherit privately from ilist_node.
> >>> Eventually I plan to either remove it entirely or move the template
> >>> parameters to the methods.
> >>>
> >>> (Note: if there's desire to bring back customization of next/prev
> >>> pointers in the future (e.g., to pack some bits in there), I think
> a
> >>> traits class like this is an awkward way to accomplish it.
> Instead,
> >>> we should change ilist<T> to be ilist<ilist_node<T>>, and give an
> >>> extra template parameter to ilist_node.)
> >>>
> >>> Modified:
> >>> llvm/trunk/include/llvm/ADT/ilist.h
> >>> llvm/trunk/include/llvm/ADT/ilist_node.h
> >>> llvm/trunk/include/llvm/IR/SymbolTableListTraits.h
> >>>
> >>> Modified: llvm/trunk/include/llvm/ADT/ilist.h
> >>> URL:
> >>> http://llvm.org/viewvc/llvm-
> >> project/llvm/trunk/include/llvm/ADT/ilist.
> >>> h?rev=278532&r1=278531&r2=278532&view=diff
> >>>
> >>
> =====================================================================
> >> =
> >>> ========
> >>> --- llvm/trunk/include/llvm/ADT/ilist.h (original)
> >>> +++ llvm/trunk/include/llvm/ADT/ilist.h Fri Aug 12 12:32:34 2016
> >>> @@ -50,20 +50,64 @@ namespace llvm { template<typename NodeTy,
> >>> typename Traits> class iplist; template<typename NodeTy> class
> >>> ilist_iterator;
> >>>
> >>> -/// ilist_nextprev_traits - A fragment for template traits for
> >>> intrusive list -/// that provides default next/prev implementations
> >> for common operations.
> >>> +/// An access class for next/prev on ilist_nodes.
> >>> ///
> >>> -template<typename NodeTy>
> >>> -struct ilist_nextprev_traits {
> >>> - static NodeTy *getPrev(NodeTy *N) { return N->getPrev(); }
> >>> - static NodeTy *getNext(NodeTy *N) { return N->getNext(); }
> >>> - static const NodeTy *getPrev(const NodeTy *N) { return
> >>> N->getPrev(); }
> >>> - static const NodeTy *getNext(const NodeTy *N) { return
> >>> N->getNext(); }
> >>> +/// This gives access to the private parts of ilist nodes. Nodes
> >> for
> >>> +an ilist /// should friend this class if they inherit privately
> >>> +from
> >> ilist_node.
> >>> +///
> >>> +/// It's strongly discouraged to *use* this class outside of ilist
> >>> +/// implementation.
> >>> +struct ilist_node_access {
> >>> + template <typename NodeTy> static NodeTy *getPrev(NodeTy *N) {
> >>> + return N->getPrev();
> >>> + }
> >>> + template <typename NodeTy> static NodeTy *getNext(NodeTy *N) {
> >>> + return N->getNext();
> >>> + }
> >>> + template <typename NodeTy> static const NodeTy *getPrev(const
> >> NodeTy *N) {
> >>> + return N->getPrev();
> >>> + }
> >>> + template <typename NodeTy> static const NodeTy *getNext(const
> >> NodeTy *N) {
> >>> + return N->getNext();
> >>> + }
> >>> +
> >>> + template <typename NodeTy> static void setPrev(NodeTy *N, NodeTy
> >> *Prev) {
> >>> + N->setPrev(Prev);
> >>> + }
> >>> + template <typename NodeTy> static void setNext(NodeTy *N, NodeTy
> >> *Next) {
> >>> + N->setNext(Next);
> >>> + }
> >>> + template <typename NodeTy> static void setPrev(NodeTy *N,
> >> std::nullptr_t) {
> >>> + N->setPrev(nullptr);
> >>> + }
> >>> + template <typename NodeTy> static void setNext(NodeTy *N,
> >> std::nullptr_t) {
> >>> + N->setNext(nullptr);
> >>> + }
> >>> +};
> >>> +
> >>> +namespace ilist_detail {
> >>>
> >>> - static void setPrev(NodeTy *N, NodeTy *Prev) { N->setPrev(Prev);
> >>> }
> >>> - static void setNext(NodeTy *N, NodeTy *Next) { N->setNext(Next);
> >>> }
> >>> +template <class T> T &make();
> >>> +
> >>> +/// Type trait to check for a traits class that has a getNext
> >>> +member (as a /// canary for any of the ilist_nextprev_traits API).
> >>> +template <class TraitsT, class NodeT> struct HasGetNext {
> >>> + typedef char Yes[1];
> >>> + typedef char No[2];
> >>> + template <size_t N> struct SFINAE {};
> >>> +
> >>> + template <class U, class V>
> >>> + static Yes &hasGetNext(
> >>> + SFINAE<sizeof(static_cast<NodeT
> >> *>(make<U>().getNext(&make<NodeT>())))>
> >>> + * = 0);
> >>> + template <class U, class V> static No &hasGetNext(...);
> >>> +
> >>> + static const bool value =
> >>> + sizeof(hasGetNext<TraitsT, NodeT>(nullptr)) == sizeof(Yes);
> >>> };
> >>>
> >>> +} // end namespace ilist_detail
> >>> +
> >>> template<typename NodeTy>
> >>> struct ilist_traits;
> >>>
> >>> @@ -93,15 +137,15 @@ struct ilist_sentinel_traits {
> >>> if (!Head) {
> >>> Head = ilist_traits<NodeTy>::createSentinel();
> >>> ilist_traits<NodeTy>::noteHead(Head, Head);
> >>> - ilist_traits<NodeTy>::setNext(Head, nullptr);
> >>> + ilist_node_access::setNext(Head, nullptr);
> >>> return Head;
> >>> }
> >>> - return ilist_traits<NodeTy>::getPrev(Head);
> >>> + return ilist_node_access::getPrev(Head);
> >>> }
> >>>
> >>> /// noteHead - stash the sentinel into its default location
> >>> static void noteHead(NodeTy *NewHead, NodeTy *Sentinel) {
> >>> - ilist_traits<NodeTy>::setPrev(NewHead, Sentinel);
> >>> + ilist_node_access::setPrev(NewHead, Sentinel);
> >>> }
> >>> };
> >>>
> >>> @@ -187,11 +231,9 @@ struct ilist_node_traits { /// By inheriting
> >>> from this, you can easily use default implementations /// for all
> >>> common operations.
> >>> ///
> >>> -template<typename NodeTy>
> >>> -struct ilist_default_traits : public
> ilist_nextprev_traits<NodeTy>,
> >>> - public
> ilist_sentinel_traits<NodeTy>,
> >>> - public ilist_node_traits<NodeTy> {
> >>> -};
> >>> +template <typename NodeTy>
> >>> +struct ilist_default_traits : public
> ilist_sentinel_traits<NodeTy>,
> >>> + public ilist_node_traits<NodeTy> {};
> >>>
> >>> // Template traits for intrusive list. By specializing this
> >> template
> >>> class, you // can change what next/prev fields are used to store
> >>> the
> >> links...
> >>> @@ -218,7 +260,6 @@ template <typename NodeTy> class
> ilist_iterator
> >>> : public std::iterator<std::bidirectional_iterator_tag, NodeTy,
> >>> ptrdiff_t> {
> >>> public:
> >>> - typedef ilist_traits<NodeTy> Traits;
> >>> typedef std::iterator<std::bidirectional_iterator_tag, NodeTy,
> >> ptrdiff_t>
> >>> super;
> >>>
> >>> @@ -279,12 +320,12 @@ public:
> >>>
> >>> // Increment and decrement operators...
> >>> ilist_iterator &operator--() {
> >>> - NodePtr = Traits::getPrev(NodePtr);
> >>> + NodePtr = ilist_node_access::getPrev(NodePtr);
> >>> assert(NodePtr && "--'d off the beginning of an ilist!");
> >>> return *this;
> >>> }
> >>> ilist_iterator &operator++() {
> >>> - NodePtr = Traits::getNext(NodePtr);
> >>> + NodePtr = ilist_node_access::getNext(NodePtr);
> >>> return *this;
> >>> }
> >>> ilist_iterator operator--(int) {
> >>> @@ -348,8 +389,11 @@ template<typename NodeTy> struct simplif
> >>> /// and the successor pointer for the sentinel (which always
> >> stays at the
> >>> /// end of the list) is always null.
> >>> ///
> >>> -template<typename NodeTy, typename Traits=ilist_traits<NodeTy> >
> >>> -class iplist : public Traits {
> >>> +template <typename NodeTy, typename Traits = ilist_traits<NodeTy>>
> >>> +class iplist : public Traits, ilist_node_access {
> >>> + static_assert(!ilist_detail::HasGetNext<Traits, NodeTy>::value,
> >>> + "ilist next and prev links are not
> customizable!");
> >>> +
> >>> mutable NodeTy *Head;
> >>>
> >>> // Use the prev node pointer of 'head' as the tail pointer. This
> >>> is really a
> >>>
> >>> Modified: llvm/trunk/include/llvm/ADT/ilist_node.h
> >>> URL:
> >>> http://llvm.org/viewvc/llvm-
> >> project/llvm/trunk/include/llvm/ADT/ilist_
> >>> node.h?rev=278532&r1=278531&r2=278532&view=diff
> >>>
> >>
> =====================================================================
> >> =
> >>> ========
> >>> --- llvm/trunk/include/llvm/ADT/ilist_node.h (original)
> >>> +++ llvm/trunk/include/llvm/ADT/ilist_node.h Fri Aug 12 12:32:34
> >>> +++ 2016
> >>> @@ -36,17 +36,13 @@ protected:
> >>> ilist_half_node() : Prev(nullptr) {} };
> >>>
> >>> -template<typename NodeTy>
> >>> -struct ilist_nextprev_traits;
> >>> -
> >>> +struct ilist_node_access;
> >>> template <typename NodeTy> class ilist_iterator;
> >>>
> >>> -/// ilist_node - Base class that provides next/prev services for
> >>> nodes -/// that use ilist_nextprev_traits or ilist_default_traits.
> >>> -///
> >>> +/// Base class that provides next/prev services for ilist nodes.
> >>> template<typename NodeTy>
> >>> class ilist_node : private ilist_half_node<NodeTy> {
> >>> - friend struct ilist_nextprev_traits<NodeTy>;
> >>> + friend struct ilist_node_access;
> >>> friend struct ilist_traits<NodeTy>;
> >>> friend struct ilist_half_embedded_sentinel_traits<NodeTy>;
> >>> friend struct ilist_embedded_sentinel_traits<NodeTy>;
> >>>
> >>> Modified: llvm/trunk/include/llvm/IR/SymbolTableListTraits.h
> >>> URL:
> >>> http://llvm.org/viewvc/llvm-
> >> project/llvm/trunk/include/llvm/IR/SymbolT
> >>> ableListTraits.h?rev=278532&r1=278531&r2=278532&view=diff
> >>>
> >>
> =====================================================================
> >> =
> >>> ========
> >>> --- llvm/trunk/include/llvm/IR/SymbolTableListTraits.h (original)
> >>> +++ llvm/trunk/include/llvm/IR/SymbolTableListTraits.h Fri Aug 12
> >>> +++ 12:32:34 2016
> >>> @@ -69,8 +69,7 @@ template <typename NodeTy> class SymbolT //
> >>> template <typename ValueSubClass> class SymbolTableListTraits
> >>> - : public ilist_nextprev_traits<ValueSubClass>,
> >>> - public SymbolTableListSentinelTraits<ValueSubClass>,
> >>> + : public SymbolTableListSentinelTraits<ValueSubClass>,
> >>> public ilist_node_traits<ValueSubClass> {
> >>> typedef SymbolTableList<ValueSubClass> ListTy;
> >>> typedef
> >>>
> >>>
> >>> _______________________________________________
> >>> llvm-commits mailing list
> >>> llvm-commits at lists.llvm.org
> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list