Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Optimize simple array comparisons (Issue 9477) #1766

Merged
merged 2 commits into from

3 participants

@CyberShadow

That was fun!

Test Results

@CyberShadow

Whoops, Phobos is failing. Not done yet...

@CyberShadow CyberShadow reopened this
@yebblies
Collaborator

It would be nice to have some more comments in there, explaining exactly what you're building.

@CyberShadow

Looks like my test has uncovered yet another new bug (unrelated to my patch)... That would be the fifth one!

@CyberShadow

Updated with Win64 test workaround and more comments.

CyberShadow added some commits
@CyberShadow CyberShadow cod2: Fix latent instruction reorder bug in cdmemcmp
Triggered on Linux by test81 in interpret.d with the
array comparison optimization patch.
c5286c3
@CyberShadow CyberShadow Optimize simple array comparison (Issue 9477) c984175
@CyberShadow

The DMD git mishap caused GitHub to screw up (it was showing others' commits that were in master anyway). Rebased on HEAD, that seems to have fixed it.

@WalterBright WalterBright merged commit 8b9d88d into from
@ghost Unknown referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@ghost Unknown referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@ghost Unknown referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@ghost Unknown referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 26, 2013
  1. @CyberShadow

    cod2: Fix latent instruction reorder bug in cdmemcmp

    CyberShadow authored
    Triggered on Linux by test81 in interpret.d with the
    array comparison optimization patch.
  2. @CyberShadow
This page is out of date. Refresh to see the latest.
Showing with 137 additions and 1 deletion.
  1. +1 −0  src/backend/cod2.c
  2. +67 −1 src/e2ir.c
  3. +69 −0 test/runnable/xtest46.d
View
1  src/backend/cod2.c
@@ -3059,6 +3059,7 @@ code *cdmemcmp(elem *e,regm_t *pretregs)
#if 1
c3 = cat(c3,getregs(mAX));
c3 = gen2(c3,0x33,modregrm(3,AX,AX)); // XOR AX,AX
+ code_orflag(c3, CFpsw); // keep flags
#else
if (*pretregs != mPSW) // if not flags only
c3 = regwithvalue(c3,mAX,0,NULL,0); // put 0 in AX
View
68 src/e2ir.c
@@ -2475,7 +2475,73 @@ elem *EqualExp::toElem(IRState *irs)
else if ((t1->ty == Tarray || t1->ty == Tsarray) &&
(t2->ty == Tarray || t2->ty == Tsarray))
{
- Type *telement = t1->nextOf()->toBasetype();
+ Type *telement = t1->nextOf()->toBasetype();
+ Type *telement2 = t2->nextOf()->toBasetype();
+
+ if ((telement->isintegral() || telement->ty == Tvoid) && telement->ty == telement2->ty)
+ {
+ // Optimize comparisons of arrays of basic types
+ // For arrays of integers/characters, and void[],
+ // replace druntime call with:
+ // For a==b: a.length==b.length && memcmp(a.ptr, b.ptr, size)==0
+ // For a!=b: a.length!=b.length || memcmp(a.ptr, b.ptr, size)!=0
+ // size is a.length*sizeof(a[0]) for dynamic arrays, or sizeof(a) for static arrays.
+
+ elem *earr1 = e1->toElem(irs);
+ elem *earr2 = e2->toElem(irs);
+ elem *eptr1, *eptr2; // Pointer to data, to pass to memcmp
+ elem *elen1, *elen2; // Length, for comparison
+ elem *esiz1, *esiz2; // Data size, to pass to memcmp
+ d_uns64 sz = telement->size(); // Size of one element
+
+ if (t1->ty == Tarray)
+ {
+ elen1 = el_una(I64 ? OP128_64 : OP64_32, TYsize_t, el_same(&earr1));
+ esiz1 = el_bin(OPmul, TYsize_t, el_same(&elen1), el_long(TYsize_t, sz));
+ eptr1 = array_toPtr(t1, el_same(&earr1));
+ }
+ else
+ {
+ elen1 = el_long(TYsize_t, ((TypeSArray *)t1)->dim->toInteger());
+ esiz1 = el_long(TYsize_t, t1->size());
+ earr1 = addressElem(earr1, t1);
+ eptr1 = el_same(&earr1);
+ }
+
+ if (t2->ty == Tarray)
+ {
+ elen2 = el_una(I64 ? OP128_64 : OP64_32, TYsize_t, el_same(&earr2));
+ esiz2 = el_bin(OPmul, TYsize_t, el_same(&elen2), el_long(TYsize_t, sz));
+ eptr2 = array_toPtr(t2, el_same(&earr2));
+ }
+ else
+ {
+ elen2 = el_long(TYsize_t, ((TypeSArray *)t2)->dim->toInteger());
+ esiz2 = el_long(TYsize_t, t2->size());
+ earr2 = addressElem(earr2, t2);
+ eptr2 = el_same(&earr2);
+ }
+
+ elem *esize = t2->ty == Tsarray ? esiz2 : esiz1;
+
+ e = el_param(eptr1, eptr2);
+ e = el_bin(OPmemcmp, TYint, e, esize);
+ e = el_bin(eop, TYint, e, el_long(TYint, 0));
+
+ if (t1->ty == Tsarray && t2->ty == Tsarray)
+ assert(t1->size() == t2->size());
+ else
+ {
+ elem *elencmp = el_bin(eop, TYint, elen1, elen2);
+ e = el_bin(op==TOKequal ? OPandand : OPoror, TYint, elencmp, e);
+ }
+
+ // Ensure left-to-right order of evaluation
+ e = el_combine(earr2, e);
+ e = el_combine(earr1, e);
+ el_setLoc(e,loc);
+ return e;
+ }
elem *ea1 = eval_Darray(irs, e1);
elem *ea2 = eval_Darray(irs, e2);
View
69 test/runnable/xtest46.d
@@ -6028,6 +6028,74 @@ void test9428()
}
/***************************************************/
+// 9477
+
+template Tuple9477(T...) { alias T Tuple9477; }
+template Select9477(bool b, T, U) { static if (b) alias T Select9477; else alias U Select9477; }
+
+void test9477()
+{
+ static bool isEq (T1, T2)(T1 s1, T2 s2) { return s1 == s2; }
+ static bool isNeq(T1, T2)(T1 s1, T2 s2) { return s1 != s2; }
+
+ // Must be outside the loop due to http://d.puremagic.com/issues/show_bug.cgi?id=9748
+ int order;
+ // Must be outside the loop due to http://d.puremagic.com/issues/show_bug.cgi?id=9756
+ auto checkOrder(bool dyn, uint expected)()
+ {
+ assert(order==expected);
+ order++;
+ // Use temporary ("v") to work around http://d.puremagic.com/issues/show_bug.cgi?id=9402
+ auto v = cast(Select9477!(dyn, string, char[1]))"a";
+ return v;
+ }
+
+ foreach (b1; Tuple9477!(false, true))
+ foreach (b2; Tuple9477!(false, true))
+ {
+ version (D_PIC) {} else // Work around http://d.puremagic.com/issues/show_bug.cgi?id=9754
+ {
+ assert( isEq (cast(Select9477!(b1, string, char[0]))"" , cast(Select9477!(b2, string, char[0]))"" ));
+ assert(!isNeq(cast(Select9477!(b1, string, char[0]))"" , cast(Select9477!(b2, string, char[0]))"" ));
+
+ assert(!isEq (cast(Select9477!(b1, string, char[0]))"" , cast(Select9477!(b2, string, char[1]))"a" ));
+ assert( isNeq(cast(Select9477!(b1, string, char[0]))"" , cast(Select9477!(b2, string, char[1]))"a" ));
+ }
+
+ assert( isEq (cast(Select9477!(b1, string, char[1]))"a", cast(Select9477!(b2, string, char[1]))"a" ));
+ assert(!isNeq(cast(Select9477!(b1, string, char[1]))"a", cast(Select9477!(b2, string, char[1]))"a" ));
+
+ assert(!isEq (cast(Select9477!(b1, string, char[1]))"a", cast(Select9477!(b2, string, char[1]))"b" ));
+ assert( isNeq(cast(Select9477!(b1, string, char[1]))"a", cast(Select9477!(b2, string, char[1]))"b" ));
+
+ assert(!isEq (cast(Select9477!(b1, string, char[1]))"a", cast(Select9477!(b2, string, char[2]))"aa"));
+ assert( isNeq(cast(Select9477!(b1, string, char[1]))"a", cast(Select9477!(b2, string, char[2]))"aa"));
+
+ // Note: order of evaluation was not followed before this patch
+ // (thus, the test below will fail without the patch).
+ // Although the specification mentions that as implementation-defined behavior,
+ // I understand that this isn't by design, but rather an inconvenient aspect of DMD
+ // that has been moved to the specification.
+ order = 0;
+ bool result = checkOrder!(b1, 0)() == checkOrder!(b2, 1)();
+ assert(result);
+ assert(order == 2);
+ }
+
+ ubyte[64] a1, a2;
+ foreach (T; Tuple9477!(void, ubyte, ushort, uint, ulong, char, wchar, dchar, float, double))
+ {
+ auto s1 = cast(T[])(a1[]);
+ auto s2 = cast(T[])(a2[]);
+ assert(s1 == s2);
+ a2[$-1]++;
+ assert(s1 != s2);
+ assert(s1[0..$-1]==s2[0..$-1]);
+ a2[$-1]--;
+ }
+}
+
+/***************************************************/
// 9504
struct Bar9504
@@ -6365,6 +6433,7 @@ int main()
test8945();
test163();
test9428();
+ test9477();
test9538();
test9700();
Something went wrong with that request. Please try again.