[llvm] r222935 - Simplify some more ownership using forward_list<T> rather than vector<unique_ptr<T>>

[David Blaikie]

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)

(obviously, once I've done this - I'll then remove the unique_ptr stuff
here, and just have a list<MatchableInfo>)


>
> >
> > >
> > > Also - did you happen to verify that your change causes the diffs on
> the AsmMatcher.inc files you mentioned to go away entirely, or were there
> other remaining changes I should hunt for/verify?
> > >
> >
> > There were some other minor diffs that might be worth looking at.
> >
> > IIRC, PR21735 describes them.
> >
> > > I don't know why this is platform-specific... I can't think of why
> > > that makes sense.
> > >
> > > Yeah, that's still rather confusing.
> > >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141222/3cded43d/attachment.html>