Skip to content
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

Minor issues related to builtins #38

Open
JamInJar opened this issue Dec 9, 2021 · 0 comments
Open

Minor issues related to builtins #38

JamInJar opened this issue Dec 9, 2021 · 0 comments

Comments

@JamInJar
Copy link

JamInJar commented Dec 9, 2021

Hi,

I am wrapping available builtins for ri5cy core pulpv3 extension instruction and by testing those have found some minor issues in the compiler related code:

  • pulp_builtins.def
    1. lines 160-163 related to mulhh[su][R]N, prototype description should use INT instead of SHORT for arguments to match corresponding md instructions descriptions, otherwise the builtins usage leads to compile time errors;
    2. lines 179-182 same for machh[us][R]N.

  • riscv-builtins.c
    1. line 339, for mac[hh][us]RN arguments check, although a norm of 0 has little interest as it becomes a simple mac[hh] the line 338 check allows it but line 339 check in such situation is machine dependent as a left shift by -1 is performed : ((1 << (Norm - 1)) == Round), a possible fix is ((Norm == 0) && (Round == 0)) || (Norm && ((1 << (Norm - 1)) == Round));
    2. line 359, same as previously but for instructions mul[hh][su]RNr, addRN[u] and subRN[u];
    3. line 398 and 408 for clip[u]_minmax, the arguments check stops with N == 29 but 30 is also a valid value, it can be reached with following fix, for (i = 0; i <= 30; i ++);
    4. lines 450-451, for invsipat1 (binsert), again although it has little interest, it is possible to insert a full 32b data at offset 0 (so to perform a move), the mask being in such a case 0xffffffff. This means that the computed size for the mask is 32 and this value is then used in a lshift in integer context so the result might be machine dependent. Also if mask is 0 and offset is also 0, no error is issued but the resulting instructions issued are hazardeous (sorry I have not captured them ...). One way to fix this check is :
    if ((MaskBar == ~Mask) && (Offset <= 31) && Mask && ((Size == 32)? (Mask == 0xffffffff) : ((unsigned int) (((1<<Size) - 1) << Offset) == Mask))) return 1;
    5. lines 463, 465, 470, 472, 477, 479, 484, 486, 491, 493, 498, 500, 505, 507, the valid CSR addresses range from 0 to 4095 ^^.

  • riscv.c
    1. lines 2587 and 2591, report clip modification to allow N == 30;
    2. line 2608, add checks :
    if (!val && !val1) return 1; if (!val) return 0;
    to align with all norm and round from immediate args modification.

All suggested modifications have been implemented and tested.

Thank you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant