[llvm] r222935 - Simplify some more ownership using forward_list<T> rather than vector<unique_ptr<T>>
David Blaikie
dblaikie at gmail.com
Mon Feb 2 11:38:16 PST 2015
On Mon, Feb 2, 2015 at 11:28 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
> > On 2015-Feb-02, at 11:00, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Mon, Jan 19, 2015 at 1:00 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> >
> > > On 2014-Dec-22, at 14:28, David Blaikie <dblaikie at gmail.com> wrote:
> > >
> > >
> > >
> > > On Fri, Dec 19, 2014 at 4:07 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > >
> > > > On 2014-Dec-19, at 16:02, David Blaikie <dblaikie at gmail.com> wrote:
> > > >
> > > >
> > > >
> > > > On Fri, Dec 19, 2014 at 2:22 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > > >
> > > > > On 2014-Dec-19, at 14:11, David Blaikie <dblaikie at gmail.com>
> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On Wed, Dec 3, 2014 at 3:57 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > > > > I looked into test/CodeGen/X86/compact-unwind.ll. llvm-mc was
> giving
> > > > > different results. Given that you changed `tablegen`, I frankly
> gave
> > > > > up on figuring out exactly why.
> > > > >
> > > > > I went back and reviewed the patch for changed semantics, and took
> a
> > > > > shot in the dark. The `sort()` at the end is different from the
> > > > > old `sort()`, because items have been front-loaded instead of back-
> > > > > loaded.
> > > > >
> > > > > The fix is to sort it backwards and then reverse the list. This
> passes
> > > > > `check` for me.
> > > > >
> > > > > Any preferences on this approach, versus using std::list (paying
> an extra pointer per element) and pushing onto the end instead? (hmm, or
> maybe we can keep an iterator to the tail end of the list and just use that
> as an insert hint while still using std::forward_list... - not sure if
> that's worth it)
> > > >
> > > > I'm not sure which approach makes sense here. How memory constrained
> > > > does `tablegen` get? `std::list<>` seems easiest if the extra
> pointer
> > > > doesn't matter.
> > > >
> > > > Welp... figured out why I wasn't seeing those failures you saw.
> std::forward_list<>::sort in libstdc++ (even in 4.9) is unstable (& in fact
> exactly inverted - which was exactly what was required). So your fix
> actually caused exactly the same failures for me as you'd seen from my
> original patch...
> > > >
> > > > In any case, that means we can't rely on its behavior in LLVM and
> I'll just use std::list<> instead. Lame.
> > > >
> > >
> > > That's a shame :(.
> > >
> > > 'tis rather.
> > >
> > > Now I have another mystery, in case anyone's interested in having a
> guess.
> > >
> > > This change:
> > >
> > > $ git diff
> > > diff --git utils/TableGen/AsmMatcherEmitter.cpp
> utils/TableGen/AsmMatcherEmitter.cpp
> > > index 3663de7..cb1f178 100644
> > > --- utils/TableGen/AsmMatcherEmitter.cpp
> > > +++ utils/TableGen/AsmMatcherEmitter.cpp
> > > @@ -613,7 +613,7 @@ public:
> > > std::forward_list<ClassInfo> Classes;
> > >
> > > /// The information on the matchables to match.
> > > - std::vector<std::unique_ptr<MatchableInfo>> Matchables;
> > > + std::list<std::unique_ptr<MatchableInfo>> Matchables;
> > >
> > > /// Info for custom matching operands by user defined methods.
> > > std::vector<OperandMatchEntry> OperandMatchInfo;
> > > @@ -1674,7 +1674,7 @@ static unsigned getConverterOperandID(const
> std::string &Name,
> > >
> > >
> > > static void emitConvertFuncs(CodeGenTarget &Target, StringRef
> ClassName,
> > > -
> std::vector<std::unique_ptr<MatchableInfo>> &Infos,
> > > +
> std::list<std::unique_ptr<MatchableInfo>> &Infos,
> > > raw_ostream &OS) {
> > > SetVector<std::string> OperandConversionKinds;
> > > SetVector<std::string> InstructionConversionKinds;
> > > @@ -2593,10 +2593,9 @@ void AsmMatcherEmitter::run(raw_ostream &OS) {
> > > // Sort the instruction table using the partial order on classes.
> We use
> > > // stable_sort to ensure that ambiguous instructions are still
> > > // deterministically ordered.
> > > - std::stable_sort(Info.Matchables.begin(), Info.Matchables.end(),
> > > - [](const std::unique_ptr<MatchableInfo> &a,
> > > - const std::unique_ptr<MatchableInfo> &b){
> > > - return *a < *b;});
> > > + Info.Matchables.sort(
> > > + [](const std::unique_ptr<MatchableInfo> &a,
> > > + const std::unique_ptr<MatchableInfo> &b) { return *a < *b;
> });
> > >
> > > DEBUG_WITH_TYPE("instruction_info", {
> > > for (const auto &MI : Info.Matchables)
> > >
> > >
> > > Causes observable differences (no test failures, but non-empty diffs)
> in the .inc files in an LLVM build.
> > >
> > > Anyone want to guess what's going on here? Maybe some partial
> instability... (unlike std::forward_list::sort's completely inverted
> instability)
> >
> > I can't think of any other explanation, unless there's UB lurking
> > somewhere.
> >
> > Sort of.
> >
> > I have a theory with some evidence:
> >
> > If I put this code:
> >
> > for (auto I = Info.Matchables.begin(), E = Info.Matchables.end(); I !=
> E; ++I) {
> > for (auto J = I; J != E; ++J) {
> > bool greater = **J < **I;
> > assert(!greater);
> > }
> > }
> >
> > immediately after the call to std::stable_sort, it does actually fail
> (even in the existing code using std::vector). (wouldn't mind someone
> verifying this, just for a sanity check - that I got the comparisons around
> the right way and that the assertion failure reproduces for them too)
> >
> > So it seems that the MatchableInfo operator< might not provide a strict
> weak ordering.
> >
> > Do you/does anyone know an easy way for me to find a specific
> contradiction in the ordering so I can understand/fix it?
> >
> > The assertion only gives me the elements that, somehow, somewhere along
> the way, std::stable_sort thought were <= but they're actually >. I want to
> find the set of elements (preferably the minimal set of elements) that
> indirectly show they are <= so I can understand which part of the
> comparison is buggy.
> >
> > Any ideas?
>
> I just had a look at the code. `MatchableInfo::operator<()` depends
> eventually on `ClassInfo::operator<()`, which includes this logic:
>
> // This class precedes the RHS if it is a proper subset of the RHS.
> if (isSubsetOf(RHS))
> return true;
>
> But the comment doesn't match the implementation of `isSubsetOf()`, which
> returns `true` even on equal sets. Is the "equal set" case possible,
> despite the pointer comparison (`this == &RHS`) above?
>
Was worth a shot, but I added an "assert(!RHS.isSubsetOf(*this))" in the if
before the return and it didn't fire, so that may not be it.
I don't think there's an immediate asymmetry (where something returns true
for both A < B and B < A) but something indirect (A < B, B < C but !(A < C)
- though it could have multiple intermediates... )
>
> I audited the rest of the code (but just quickly) and nothing else popped
> out.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150202/4df9ea03/attachment.html>
More information about the llvm-commits
mailing list