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

Generate a sequence of instructions for divisions on Arm targets #126

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

DrXiao
Copy link
Collaborator

@DrXiao DrXiao commented Mar 28, 2024

Division instruction doesn't exist in ARMv7-A (#46 ), but the current implementation of shecc still generates it to compute the result for division or modulo operations.

Thus, here adjusts arm-codegen so that shecc will generate an instruction sequence to achieve integer division without relying on any division instruction.

@jserv jserv requested a review from vacantron March 28, 2024 14:19
Copy link
Collaborator

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Add new command line option m, which indicates the use of hardware integer multiplication and division instructions.
  2. Extend tests/driver.sh for more integer division coverage.

@DrXiao
Copy link
Collaborator Author

DrXiao commented Mar 28, 2024

@jserv I understand that we can add a m option to determine whether shecc generates hardware integer multiplication and division instructions.

However, I consider shecc can still generate multiplication instructions for ARM even if the m option is closed. Because ARMv7-A inherently includes multiplication instruction, we can generate it directly without any concern.

Therefore, for the implementation of the m option, I think that I can just handle whether the hardware integer division instructions are generated for ARM.

@jserv
Copy link
Collaborator

jserv commented Mar 28, 2024

However, I consider shecc can still generate multiplication instructions for ARM even if the m option is closed. Because ARMv7-A inherently includes multiplication instruction, we can generate it directly without any concern.
Therefore, for the implementation of the m option, I think that I can just handle whether the hardware integer division instructions are generated for ARM.

Yes for Arm backend. Please proceed.

@DrXiao
Copy link
Collaborator Author

DrXiao commented Mar 29, 2024

The reference implementation mentioned in #46 assumes that the dividend and divisor are unsigned integers, but it may fail if at least one of them is a signed integer.

Can I suppose that shecc will not encounter any signed integer division and just handle unsigned integer division?
Or do we must let shecc handle both of them?

@jserv jserv changed the title Generate an instruction sequence for division on ARM targets. Generate a sequence of instructions for implementing division on Arm targets Mar 29, 2024
@jserv jserv changed the title Generate a sequence of instructions for implementing division on Arm targets Generate a sequence of instructions for divisions on Arm targets Mar 29, 2024
@jserv
Copy link
Collaborator

jserv commented Mar 29, 2024

Can I suppose that shecc will not encounter any signed integer division and just handle unsigned integer division? Or do we must let shecc handle both of them?

Given your aim to replace the current udiv/sdiv instruction, the proposed changes should also ensure division is covered for both signed and unsigned integers, mirroring the approach gcc took with its libgcc.

@jserv
Copy link
Collaborator

jserv commented Mar 29, 2024

By the way, I enjoy this article Fast(est) Double-Word Integer Division.

@DrXiao
Copy link
Collaborator Author

DrXiao commented Mar 31, 2024

I have a new question about the right shift operation. During these days, I test the following code:

/* test.c */
int main() {
    printf("%d\n", -2 >> 31);
    return 0;
}

In C99, It mentions that it is an implementation-defined behavior for using right shift operation on signed integers.

If we use gcc and clang to compile and execute it, we can all obtain the output -1:

$ gcc -o test test.c
$ ./test
-1
$ clang -o test test.c
$ ./test
-1

I believe it may be a convention that many compilers use arithmetic right shift instructions for signed integers.

But, when using shecc to test the right shift operation, we will get the output 1 because shecc uses logical right shift instructions for signed integers:

$ qemu-arm out/shecc-stage2.elf -o test test.c
$ qemu-arm test
1

Should we instruct shecc to generate arithmetic right shift instructions as well?

@jserv
Copy link
Collaborator

jserv commented Mar 31, 2024

In C99, It mentions that it is an implementation-defined behavior for using right shift operation on signed integers.
[...]
Should we instruct shecc to generate arithmetic right shift instructions as well?

@vacantron, can you comment this?

@DrXiao DrXiao requested a review from jserv March 31, 2024 15:23
jserv

This comment was marked as resolved.

@vacantron
Copy link
Collaborator

In C99, It mentions that it is an implementation-defined behavior for using right shift operation on signed integers.
[...]
Should we instruct shecc to generate arithmetic right shift instructions as well?

Yes, the behavior should be consistent with GCC or Clang.

Thank you for the report!

@jserv
Copy link
Collaborator

jserv commented Apr 2, 2024

I defer to @vacantron for his performance evaluation.

Copy link
Collaborator

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase the latest master branch for bool specific fixes.

@DrXiao
Copy link
Collaborator Author

DrXiao commented Apr 5, 2024

I encounter a new issue when rebasing:

$ make
  CC    out/src/main.o
  LD    out/shecc
  SHECC out/shecc-stage1.elf
munmap_chunk(): invalid pointer
Aborted (core dumped)
make: *** [Makefile:80: out/shecc-stage1.elf] Error 134

Then, I checked the source code including my changes, and found that shecc has a potential problem:

$ wc --bytes src/* out/libc.inc
  8120 src/arm.c
 15909 src/arm-codegen.c
 15909 src/codegen.c
  8889 src/defs.h
  8052 src/elf.c
 22550 src/globals.c
 16317 src/lexer.c
  2526 src/main.c
 94104 src/parser.c
  1716 src/peephole.c
 25695 src/reg-alloc.c
  8952 src/riscv.c
 11660 src/riscv-codegen.c
 38460 src/ssa.c
 25330 out/libc.inc
304189 total

It can be observed that the total bytes of all source files have exceeded 278528, as defined by MAX_SOURCE.
Even after reducing the source about RISC-V or Arm, the total bytes of the utilized source files still approximate 280000.

This will cause the SOURCE variable not to store all bytes of the sources and the executable may access invalid memory regions.

Should I enlarge the allocated size in this PR to prevent the potential problem?

@vacantron
Copy link
Collaborator

Should I enlarge the allocated size in this PR to prevent the potential problem?

Yes, please do so.

Division instruction doesn't exist in ARMv7-A, but the current implementation
of shecc still generates it to compute the result for division or modulo
operations.

Thus, here adjusts arm-codegen so that shecc will generate a sequence of
instructions to achieve signed integer division without relying on any division
instruction.

Additionally, extend test/driver.sh to test the divisions and add a new command
line option ' m' to determine whether generating hardware multiplication and
division instructions for the executable compiled by shecc:

$ shecc [-o output] [ m] [--no-libc] [--dump-ir] <infile.c>

Finally, the allocated size for 'SOURCE', used to store all bytes of the source
files, has been enlarged to prevent access to invalid memory regions.
@vacantron
Copy link
Collaborator

I defer to @vacantron for his performance evaluation.

Because of #120 , the measurement of the performance is postponed. I prefer to apply this patch first.

@jserv jserv merged commit 86ddfc5 into sysprog21:master Apr 5, 2024
4 checks passed
@jserv
Copy link
Collaborator

jserv commented Apr 5, 2024

Thank @DrXiao for contributing!

@DrXiao
Copy link
Collaborator Author

DrXiao commented Apr 5, 2024

@jserv May I ask why this PR can be merged? Or will this PR be reopened and requested to be improved again in the future?

@jserv
Copy link
Collaborator

jserv commented Apr 5, 2024

May I ask why this PR can be merged? Or will this PR be reopened and requested to be improved again in the future?

The latter.

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

Successfully merging this pull request may close these issues.

3 participants