-
Notifications
You must be signed in to change notification settings - Fork 151
use tg declarations to handle mat and vec dimensions #1281
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@wsmoses are we sure that this is correct? Enzyme/enzyme/Enzyme/Utils.cpp Line 2103 in c5a6954
I just added handling for transposed matrices and my tests indicate that it's not. I did modify the gemm_f_c_transpose_lacpy.ll testcase by setting transa to 'T' instead of 'N' (and A is modified, thus cached). In the T case the lacpy args to cache A should be switched as compared to the old 'N" testcase, but that's not happening. Edit. sry, nvm. mixed up tests. Caching now works correct under transpositions. |
|
That code is wrong there should never be a builder nor any load or store
generated
…On Thu, Jun 15, 2023 at 8:00 AM Manuel Drehwald ***@***.***> wrote:
@wsmoses <https://github.com/wsmoses> are we sure that this is correct?
https://github.com/EnzymeAD/Enzyme/blob/c5a6954570b6929ac10cdd7f6c8b3c505ecab82c/enzyme/Enzyme/Utils.cpp#L2103
I just added handling for transposed matrices and my tests indicate that
it's not.
I did modify the gemm_f_c_transpose_lacpy.ll testcase by setting transa to
'T' instead of 'N' (and A is modified, thus cached).
—
Reply to this email directly, view it on GitHub
<#1281 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJTUXFVFNNEUMJSLAHLWZLXLKQGLANCNFSM6AAAAAAZHI5GZM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
The condition at the end should be return false at the end of the function
not return true
…On Thu, Jun 15, 2023 at 8:04 AM Billy Moses ***@***.***> wrote:
That code is wrong there should never be a builder nor any load or store
generated
On Thu, Jun 15, 2023 at 8:00 AM Manuel Drehwald ***@***.***>
wrote:
> @wsmoses <https://github.com/wsmoses> are we sure that this is correct?
>
> https://github.com/EnzymeAD/Enzyme/blob/c5a6954570b6929ac10cdd7f6c8b3c505ecab82c/enzyme/Enzyme/Utils.cpp#L2103
> I just added handling for transposed matrices and my tests indicate that
> it's not.
> I did modify the gemm_f_c_transpose_lacpy.ll testcase by setting transa
> to 'T' instead of 'N' (and A is modified, thus cached).
>
> —
> Reply to this email directly, view it on GitHub
> <#1281 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAJTUXFVFNNEUMJSLAHLWZLXLKQGLANCNFSM6AAAAAAZHI5GZM>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
|
There should also be a continue after the for loop adding all the users
into todo
…On Thu, Jun 15, 2023 at 8:06 AM Billy Moses ***@***.***> wrote:
The condition at the end should be return false at the end of the function
not return true
On Thu, Jun 15, 2023 at 8:04 AM Billy Moses ***@***.***>
wrote:
> That code is wrong there should never be a builder nor any load or store
> generated
>
> On Thu, Jun 15, 2023 at 8:00 AM Manuel Drehwald ***@***.***>
> wrote:
>
>> @wsmoses <https://github.com/wsmoses> are we sure that this is correct?
>>
>> https://github.com/EnzymeAD/Enzyme/blob/c5a6954570b6929ac10cdd7f6c8b3c505ecab82c/enzyme/Enzyme/Utils.cpp#L2103
>> I just added handling for transposed matrices and my tests indicate that
>> it's not.
>> I did modify the gemm_f_c_transpose_lacpy.ll testcase by setting transa
>> to 'T' instead of 'N' (and A is modified, thus cached).
>>
>> —
>> Reply to this email directly, view it on GitHub
>> <#1281 (comment)>,
>> or unsubscribe
>> <https://github.com/notifications/unsubscribe-auth/AAJTUXFVFNNEUMJSLAHLWZLXLKQGLANCNFSM6AAAAAAZHI5GZM>
>> .
>> You are receiving this because you were mentioned.Message ID:
>> ***@***.***>
>>
>
|
|
@wsmoses thanks. Behaves better now. |
Co-authored-by: William Moses gh@wsmoses.com
|
This now also has enough to remove this one: |
|
this broke both macOS build and macOS tests |
|
weren't they broken already for a while @tgymnich ? |
|
Yeah i fixed them last WE. The Xcode build is the only one still broken (but thats just the cmake arg order). |
No description provided.