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

Duncan P. N. Exon Smith dexonsmith at apple.com
Thu Feb 5 11:50:29 PST 2015


> On 2015-Feb-04, at 22:21, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>> 
>> On 2015 Feb 2, at 11:38, David Blaikie <dblaikie at gmail.com> wrote:
>> 
>> 
>> 
>> 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.
>>> 
> 
> Okay, so back to your previous question: it sounds like you're looking
> for the shortest cycle in a directed graph (where the nodes are
> `MatchableInfo*` with edges `L -> R` when `MatchableInfo::Less(L, R)`).

The `findShortestCycle()` code in the attached patch should get you what
you want (although as you can see I've hardly tested it).  Get the
shortest cycle like this:

    std::vector<unsigned> Cycle =
        findShortestCycle(Matchables.size(), [&](unsigned T, unsigned H) {
          return MatchableInfo::Less()(Matchables[T], Matchables[H]);
        });

-------------- next part --------------
A non-text attachment was scrubbed...
Name: shortest-cycle.patch
Type: application/octet-stream
Size: 3912 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150205/11cf2a62/attachment.obj>
-------------- next part --------------

> 
>>> 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.
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits



More information about the llvm-commits mailing list