[llvm] r222935 - Simplify some more ownership using forward_list<T> rather than vector<unique_ptr<T>>
Craig Topper
craig.topper at gmail.com
Mon Feb 2 11:37:26 PST 2015
On Mon, Feb 2, 2015 at 11:00 AM, 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?
>
Well I know for a fact that Variant isn't checked at all in the comparison.
There are also a lot of failures if you try to enable the ambiguous match
checking.
>
>
>> 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.
>> > > >
>> > >
>> >
>>
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
--
~Craig
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150202/7e40e418/attachment.html>
More information about the llvm-commits
mailing list