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

[Duncan P. N. Exon Smith]

> 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.  This patch looks obviously correct to me :/.

> (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.
> > >
> >
>