0

I am writing an assembly module for my c++ program. Procedure f_sin calculates sine of some value that is stored in one array and puts result in other array. But when I run my program, sine array seems to be filled with garbage values. I can't really get where it fails.

I'm using Visual Studio 2019.

main.cpp

#include<iostream>
extern "C" void f_sin(int* res, int N, double* sine);

int main() {
    int N = 3;
    int arr[N] = {1, 2, 3};
    double sine[3];

    f_sin(arr, N, sine);

    for (int i = 0; i < N; i++)
        std::cout << sine[i] << " ";

    return 0;
}

proc.asm

.586p
.MODEL FLAT, C
.CODE
PUBLIC C f_sin
f_sin PROC C arr: dword, Nint: dword, sinarr: dword

push esi
push edi
push ebp

mov edi, sinarr
mov esi, arr
mov ecx, Nint

count:
    finit
    fld dword ptr [esi]
    fsin
    fstp dword ptr [edi]
    add edi, 4
    add esi, 4
    loop count

pop ebp
pop edi
pop esi

ret
f_sin ENDP
END
Mgetz
  • 5,108
  • 2
  • 33
  • 51
randomsquare
  • 33
  • 6
  • i am not able to read assembler, but `int N=5;` is suspicious. What is `N` ? – 463035818_is_not_an_ai Dec 21 '21 at 13:21
  • @463035818_is_not_a_number forgot to edit my code – randomsquare Dec 21 '21 at 13:24
  • If your inputs are integers, you likely want degrees not radians so do a conversion. – Jester Dec 21 '21 at 13:29
  • @randomsquare what is your project in VS set to use as the default calling convention? [`__stdcall`](https://learn.microsoft.com/en-us/cpp/cpp/stdcall?view=msvc-170) or [`__cdecl`](https://learn.microsoft.com/en-us/cpp/cpp/cdecl?view=msvc-170)? – Mgetz Dec 21 '21 at 13:31
  • 4
    A `double` is 8 bytes, not 4. – Mark Ransom Dec 21 '21 at 13:41
  • @Mgetz it's `_cdecl`. – randomsquare Dec 21 '21 at 13:41
  • @randomsquare if it's `__cdecl` you need to grab your parameters from the stack *"Pushes parameters on the stack, in reverse order (right to left)"* they won't be in registers – Mgetz Dec 21 '21 at 13:45
  • @MarkRansom so I should use `qword` instead of `dword` for `sine` array? – randomsquare Dec 21 '21 at 14:12
  • 1
    Or I just use `float` instead of `double`. That worked. Thanks everyone! – randomsquare Dec 21 '21 at 14:15
  • 1
    The push and pop of `ebp` seems suspicious (though not a problem in practise). You aren't using or setting `ebp` by yourself anywhere. If the assembler sets up `ebp` to access the parameters then you don't need to push and pop it explicitly. – ecm Dec 21 '21 at 14:36
  • You don't need `finit` inside the loop (or at all, since you balance push/pop correctly, unless your compiler's CRT startup code reduces the x87 precision setting or something). Your input is integer data, you need `fild`. The `loop` instruction is slow, avoid it. You could use EAX and EDX, not ESI and EDI, so you don't need to save/restore any call-preserved registers. – Peter Cordes Dec 21 '21 at 19:54
  • Also, if you want to do `sin` of a whole array, you could use SIMD instead of going one at a time. There is no hardware/microcode `sinps` instruction, but there are vector math library functions whose asm you could look. See [C++ error: ‘\_mm\_sin\_ps’ was not declared in this scope](https://stackoverflow.com/q/31978592) for some links, and https://github.com/vectorclass/version2/blob/master/vectormath_trig.h – Peter Cordes Dec 21 '21 at 19:56
  • You'll probably want to profile and test `fsin` before assuming that it's a useful way to speed up your program. Although it's a single instruction, it's a very expensive one (20+ uops says uops.info, and throughput of 1 per 8 cycles), and has some significant accuracy problems. See https://stackoverflow.com/a/1845204/634919 for more. It exists mainly for legacy programs and isn't really meant to be used in new code. – Nate Eldredge Dec 22 '21 at 00:05

0 Answers0