FEniCS / ffcx

Next generation FEniCS Form Compiler for finite element forms
https://fenicsproject.org
Other
144 stars 38 forks source link

Avoid nesting conditions and statements with many operands #651

Closed IgorBaratta closed 7 months ago

IgorBaratta commented 7 months ago

This PR fixes issue #652.

Also it simplifies how we create statements with LNodes in the integral generator. Before this PR, each statement could be nested and could be understood as a tree of statements:

A = OP1(OP2(X, OP3(Y, Z)), W)

With this PR we create statements with 2 operands and 1 destination:

A0 = OP3(Y, Z)
A1 = OP2(X, A0)
A2 = OP1(A1, W)
michalhabera commented 7 months ago

So this change disables the intermediate array storage, instead, all intermediates go to new local variable. This is certainly better for varying data types of intermediate symbols. I think the old array like storage was probably motivated by performance, but I cannot see why this argument should hold. Did you benchmark it?

IgorBaratta commented 7 months ago

So this change disables the intermediate array storage, instead, all intermediates go to new local variable. This is certainly better for varying data types of intermediate symbols. I think the old array like storage was probably motivated by performance, but I cannot see why this argument should hold. Did you benchmark it?

Yes, for most forms the C compiler generates the same code for the array like storage and scalar variables. In other cases the compiler can vectorize the code with local variables (SLP vectorization) but not with the array.

michalhabera commented 7 months ago

So this change disables the intermediate array storage, instead, all intermediates go to new local variable. This is certainly better for varying data types of intermediate symbols. I think the old array like storage was probably motivated by performance, but I cannot see why this argument should hold. Did you benchmark it?

Yes, for most forms the C compiler generates the same code for the array like storage and scalar variables. In other cases the compiler can vectorize the code with local variables (SLP vectorization) but not with the array.

Right, SLP vectorizer probably does not work with arrays, that's is a good point. I've never seen SLP do anything useful in our codes in the past, but that was most likely due to the opaque array. I think this is a bugfix with a change in the right direction ;)

coveralls commented 7 months ago

Coverage Status

coverage: 80.367% (+0.03%) from 80.34% when pulling 0d23e0bd332233fe7c5f4bef97982b73d736d177 on igor/remove-opt into c366395d0af10652cd60642f82457cfac378996a on main.