add the possibility to check the IL in tests #145

jogibear9988 · 2018-09-07T22:29:32Z

No description provided.

dadhi · 2018-09-08T04:36:13Z

Great idea!

dadhi · 2018-09-08T05:16:17Z

test/FastExpressionCompiler.IssueTests/Issue91_Issue95_Tests.cs

+
+            // Check TryEmitInvertedNullComparison is used
+            var il = ILReaderFactory.Create(convert1.Method).ToList();
+            Assert.AreEqual(il[0].OpCode, OpCodes.Ldarg_0);


May be even simplified by CollectionAssert.AreEqual()

jogibear9988 · 2018-09-08T16:55:06Z

I'm still workin on one more linq2db test.

But there is see, MsCompiler can also line parameterd lambdas

jogibear9988 · 2018-09-08T17:45:16Z

I've now a Problem with ldarga.s

In the Lambda a Method is Called "ConvertString" wich gets a Int as a Parameter wich is a argument.

Our Code now emits ldarga.s but Microsoft code emits ldarg.s

                 var asAddress = (parent == ExpressionType.Call || parent == ExpressionType.MemberAccess) && paramExpr.Type.IsValueType() && !paramExpr.IsByRef;

This now returns true but should return false, any idea how to fix?

jogibear9988 · 2018-09-08T17:50:05Z

I've added a test

jogibear9988 · 2018-09-08T18:01:43Z

I've added a fix, don't know if it's correct ;-(

And if it's correct I think we need to hand over this parameter through every try emit call (like the others)

dadhi · 2018-09-08T18:31:04Z

And if it's correct I think we need to hand over this parameter through every try emit call (like the others)

I have started the work on combining all this parameters into enum flags. So far, so good. Much easy to trace the code and find the patterns - and simplify some parts.

So this new parameter will be just an another flag, no need to change parameters through all calls.

Plus I have started to inline things were possible. e.g. TryEmitMany and TryEmitBinary. It localize the code checks, in some places they even not required anymore.

Will see how it goes.

jogibear9988 · 2018-09-08T18:37:07Z

do not yet merge, I'm workin on an issue with struct types when ldflda instead of lddld has tu be used. But I need to find out when

dadhi · 2018-09-08T19:19:02Z

Won't do. May be will ask you for rebase when I done with my changes.

jogibear9988 · 2018-09-09T09:50:21Z

Info when to use Ldloc vs LdlocA https://stackoverflow.com/questions/4273331/ldloc-or-ldloca

jogibear9988 · 2018-09-09T19:31:22Z

Only one Linq2db test atm now fails on my system.

But 3 times I thought now I had the problem and it's still there. (It's a huge Expression, so the 3 problems were errors we had, but there are still some left)

jogibear9988 · 2018-09-10T19:23:00Z

@dadhi how far did you get with your refactoring?

dadhi · 2018-09-10T19:44:08Z

@dadhi how far did you get with your refactoring?

I have finished mechanically, but a big amount of test fails. Will try to fix, and optimize my refactoring tomorrow.

dadhi · 2018-09-11T09:51:11Z

@jogibear9988
I have finished my refactoring.
Summing it up:

I have introduced extensible ParentFlags enum and corresponding parent parameter in TryEmit.. replacing old parent, ignoreResult, isMemberAccess parameters.
TryEmit.. methods where inlined where possible to minimize call-stack, as a result, TryEmitMany and TryEmitBinary disappeared. Other small methods were inlined too.
Type expr parameter was removed from TryEmit.. as easily acquired from expression.

The problem will be to merge this PR, cause some methods were inlined.
Maybe just go change by change commit, re-adding them together with tests.

jogibear9988 · 2018-09-11T11:54:06Z

i look to fix tonight

jogibear9988 · 2018-09-11T21:46:40Z

src/FastExpressionCompiler/FastExpressionCompiler.cs

+                    var byRefIndex = argExprs[i].Type.IsByRef ? i : -1;
+                    if (!TryEmit(argExprs[i], paramExprs, il, ref closure, parent & ~ParentFlags.IgnoreResult, byRefIndex))
                        return false;
+                }


@dzmitry-lahoda do you think this is a correct fix ?

dadhi · 2018-09-12T03:52:26Z

src/FastExpressionCompiler/FastExpressionCompiler.cs

            IgnoreResult = 1 << 1,
            Call = 1 << 2,
-            MemberAccess = 1 << 3,
+            MemberAccess = 1 << 3, // Any Parent Expression is a MemberExpression


Regarding your comment here, parent flags currently are propagated through nested expressions until changed. That means it is more of an ancestor than a parent.

But we may reset the parent in TryEmit when we go deeper. It was my initial variant. Then I removed the reset and nothing changed. So if you found test where it does not work, we may need return reset back:

internal static ParentFlags Reset(ParentFlags p) => p & (ParentFlags.IgnoreResult);

Should we preserve IsInstanceAccess as well?

dadhi · 2018-09-12T03:55:26Z

src/FastExpressionCompiler/FastExpressionCompiler.cs


            public bool HasClosure => ClosureType != null;

+            public bool LastEmitIsAddress;


Can we make it into ParentFlags?

No, it's used from the Caller

dadhi · 2018-09-12T04:17:10Z

src/FastExpressionCompiler/FastExpressionCompiler.cs

+                            var notExpr = (UnaryExpression)expr;
+                            if (!TryEmit(notExpr.Operand, paramExprs, il, ref closure, parent))
+                                return false;
+                            if ((parent & ParentFlags.IgnoreResult) > 0)


This IL emit may be moved to a method.

dadhi · 2018-09-12T04:18:20Z

src/FastExpressionCompiler/FastExpressionCompiler.cs


                    var asAddress = 
                        paramType.IsValueType() && !paramExpr.IsByRef &&
-                        (parent & (ParentFlags.Call | ParentFlags.MemberAccess)) != 0;


Oops, my fail :(
Thanks for fixing.

dadhi · 2018-09-12T04:19:20Z

src/FastExpressionCompiler/FastExpressionCompiler.cs

+                        var locT = il.DeclareLocal(targetType);
+                        il.Emit(OpCodes.Stloc_S, loc);
+                        il.Emit(OpCodes.Ldloca_S, loc);
+                        var hasValueMethod = sourceTypeInfo.GetDeclaredMethod("get_HasValue");


I have a method for this in Tools.

FindNullableHasValueGetterMethod or something

dadhi · 2018-09-12T04:20:08Z

src/FastExpressionCompiler/FastExpressionCompiler.cs

+                            return false;
+                        il.Emit(OpCodes.Brfalse, labelFalse);
+                        il.Emit(OpCodes.Ldloca_S, loc);
+                        var mthValue = sourceTypeInfo.GetDeclaredMethods("GetValueOrDefault")


..the same, there is method in Tools.

FindNullableGetValueOrDefaultMethod or something

jogibear9988 · 2018-09-12T06:05:41Z

I still have a bug in the Linq2db Tests, wich I need to find.

So do not merge yet

dadhi · 2018-09-12T06:18:59Z

ok

jogibear9988 · 2018-09-12T08:56:50Z

src/FastExpressionCompiler/FastExpressionCompiler.cs

+                                var idx = argExprs[i].Type.IsByRef ? i : -1;
+                                if (!TryEmit(argExprs[i], paramExprs, il, ref closure, parent, idx))
                                    return false;
+                            }


@dzmitry-lahoda again, is this correct fix for new with byref?

yep. Originally I missed that in .NET there is more then one way to call method (usual, and via delegate and via ctor).

dadhi · 2018-09-12T10:16:22Z

the following 2 Il commands are missing in FEC

The code is required because of System.Compile represents Closure as object[] in this case.
To load value type DateTime from the object array, it loads the index of an array, then loads the reference to an element in the array by index, and because the element is an object, it unboxes it into DateTime.

FEC up to some number of items represents closure as typed Closure<T1, ... Tn> object. Therefore the field load is enough.

But after some amount of items, FEC starts to use ArrayClosure which is the same object[] wrapped in the class. So we may need to check this case.

Here the code in constructing closure which decides what to use:

FastExpressionCompiler/src/FastExpressionCompiler/FastExpressionCompiler.cs

Lines 441 to 460 in 49294ea

    
           var createMethods = ExpressionCompiler.Closure.CreateMethods; 
        
           if (totalItemCount > createMethods.Length) 
        
           { 
        
               ClosureType = typeof(ArrayClosure); 
        
               if (constructTypeOnly) 
        
                   return null; 
        
               var items = new object[totalItemCount]; 
        
               if (constants.Length != 0) 
        
                   for (var i = 0; i < constants.Length; i++) 
        
                       items[i] = constants[i].Value; 
        
               // skip non passed parameters as it is only for nested lambdas 
        
               if (nestedLambdas.Length != 0) 
        
                   for (var i = 0; i < nestedLambdas.Length; i++) 
        
                       items[constPlusParamCount + i] = nestedLambdas[i].Lambda; 
        
               return new ArrayClosure(items); 
        
           }

You may comment the methods here to enforce ArrayClosure usage and check the tests.

… are not found in closure

dadhi · 2018-09-14T06:02:05Z

@jogibear9988 Hi, it is an amazing thread of work.
Can we merge it now, cause it becomes harder to check and review all things here?

jogibear9988 · 2018-09-14T07:45:16Z

You can merge.

linq2db Tests now work except 3, this 3 did also not work before your refactoring. I#m still looking to get them working, but it's a huge expression tree wich I need to pack into a test!

jogibear9988 · 2018-09-14T07:46:09Z

You also then can close the other 2 pull req. I've included them in mine and fixed the issues

add the possibility to check the IL in tests

81ccd75

dadhi reviewed Sep 8, 2018

View reviewed changes

jogibear9988 added 3 commits September 8, 2018 07:31

use collectionassert

ddb5b4e

check multiple constant returns are removed

1e03dc7

Nullable -> Nullable convert

db67a26

test for ldarg error

9e5ddde

bugfix ladarg

e00efb3

jogibear9988 added 2 commits September 9, 2018 00:11

fixes for linq2db

62557ff

bugfixes for linq2db tests

61de7c1

jogibear9988 added 2 commits September 9, 2018 19:52

more tests, more fixes

b64b296

support convert checked

9485794

jogibear9988 added 5 commits September 9, 2018 21:50

bugfix more for linq2db

27c43af

bugfix implement not

8335feb

try to reproduce 146

69db8c5

more test for 146

8984a48

work on issue dadhi#147

8392052

jogibear9988 commented Sep 11, 2018

View reviewed changes

dadhi reviewed Sep 12, 2018

View reviewed changes

jogibear9988 added 3 commits September 12, 2018 10:12

support custom expression nodes

b5ed5fc

support reduce

c43fe1b

bugfix byref by new

3bb21ab

jogibear9988 commented Sep 12, 2018

View reviewed changes

jogibear9988 added 3 commits September 13, 2018 14:24

support decimal

933b66e

bugfix fix dadhi#153 const access

2a65959

bugfix 153

1889265

jogibear9988 force-pushed the patch18 branch from 74a1979 to 1889265 Compare September 13, 2018 16:21

jogibear9988 added 2 commits September 13, 2018 19:16

fix rest of linq2db tests

a451680

comment for decimal

05533ce

jogibear9988 changed the title ~~add the possibility to check the IL in tests~~ [do not merge - yet] add the possibility to check the IL in tests Sep 13, 2018

jogibear9988 added 6 commits September 13, 2018 21:33

bugfix equality

3bba396

bugfix reduced expressions, we should only reduce once, or parameters…

b83e807

… are not found in closure

bugfix support string +

58ff50e

support typeas

ef868a6

bugfix nullable arithmetic

61c1cf0

support typeis

ba85df0

jogibear9988 changed the title ~~[do not merge - yet] add the possibility to check the IL in tests~~ add the possibility to check the IL in tests Sep 14, 2018

dadhi merged commit 11191a4 into dadhi:master Sep 14, 2018

This was referenced Sep 14, 2018

Adding failing test for lambda () => int.MinValue #152

Closed

'Attempt to read protected memory' #150

Closed


		public bool HasClosure => ClosureType != null;

		public bool LastEmitIsAddress;

Uh oh!

add the possibility to check the IL in tests #145

add the possibility to check the IL in tests #145

Uh oh!

Conversation

jogibear9988 commented Sep 7, 2018

Uh oh!

dadhi commented Sep 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jogibear9988 commented Sep 8, 2018

Uh oh!

jogibear9988 commented Sep 8, 2018

Uh oh!

jogibear9988 commented Sep 8, 2018

Uh oh!

jogibear9988 commented Sep 8, 2018

Uh oh!

dadhi commented Sep 8, 2018

Uh oh!

jogibear9988 commented Sep 8, 2018

Uh oh!

dadhi commented Sep 8, 2018

Uh oh!

jogibear9988 commented Sep 9, 2018

Uh oh!

jogibear9988 commented Sep 9, 2018

Uh oh!

jogibear9988 commented Sep 10, 2018

Uh oh!

dadhi commented Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dadhi commented Sep 11, 2018

Uh oh!

jogibear9988 commented Sep 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jogibear9988 commented Sep 12, 2018

Uh oh!

dadhi commented Sep 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dadhi commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dadhi commented Sep 14, 2018

Uh oh!

jogibear9988 commented Sep 14, 2018

Uh oh!

jogibear9988 commented Sep 14, 2018

Uh oh!

Reviewers

dadhi commented Sep 10, 2018 •

edited

Loading

dadhi commented Sep 12, 2018 •

edited

Loading