Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive study note document that explores the intricate ways modern compilers optimize integer division operations, particularly when the divisor is a power of two. It delves into the architectural specifics of x86 and AArch64, demonstrating how signed and unsigned division are transformed into efficient bitwise shifts and conditional logic to maintain C's defined behavior, offering valuable insights into low-level compiler optimizations. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds study notes for Day 6 of Advent of Compiler Optimisations 2025, covering division optimizations. I've identified several issues that need correction:
Critical Issues:
- Logic error in line 160: The shift direction for
shrlinstruction is documented incorrectly as left shift (<<) when it should be right shift (>>) - Incorrect instruction name on line 132: References
cmovensinstead ofcmovns - Incorrect register on line 209: Comment states
w8when it should referencew0
Minor Issues:
- Three typos: "consdier" → "consider" (line 108), "filp" → "flip" (line 119), "Unsign" → "Unsigned" (line 136)
Please address these corrections to ensure the technical accuracy of the documentation.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
|
|
||
| Instruction: | ||
| ``` | ||
| shrl <imm>, <Rd> ; shr := Shift Right Logical, that is <Rd> = <Rd> << <imm> |
There was a problem hiding this comment.
🛑 Logic Error: The shift direction is incorrect. shrl performs a right shift (>>), not a left shift (<<). This documentation error could mislead readers about how the instruction works.
| shrl <imm>, <Rd> ; shr := Shift Right Logical, that is <Rd> = <Rd> << <imm> | |
| shrl <imm>, <Rd> ; shr := Shift Right Logical, that is <Rd> = <Rd> >> <imm> |
| Instructions: | ||
| ``` | ||
| - add <Wd>, <Wn>, #imm ; w8 = w0 + 0x1ff | ||
| - cmp <Wn>, #imm ; Compares w8 with #0x0, and update the processor flags NZCV |
There was a problem hiding this comment.
Incorrect register referenced in comment. The cmp instruction compares w0 (not w8) with #0x0.
| - cmp <Wn>, #imm ; Compares w8 with #0x0, and update the processor flags NZCV | |
| - cmp <Wn>, #imm ; Compares w0 with #0x0, and update the processor flags NZCV |
|
|
||
| Hence, the compiler does `(-1 + 511) / 512 = 510 / 512 = 510 >> 9 = 0`, we get the correct result. | ||
|
|
||
| In summary, the compiler use the `cmovens` and `test` instructions to detect whether `x` is positive. |
There was a problem hiding this comment.
Incorrect instruction name. The assembly output shows cmovnsl (Conditional Move if Not Sign), not cmovens.
| In summary, the compiler use the `cmovens` and `test` instructions to detect whether `x` is positive. | |
| In summary, the compiler use the `cmovns` and `test` instructions to detect whether `x` is positive. |
| To resolve this problem, the compiler adds `2^n - 1` to the negative number. | ||
| In this case, n = 9, hence it is `2^9 - 1 = 512 - 1 = 511 = 0x1FF` | ||
|
|
||
| Why `2^n - 1` ? Let's consdier `n = 9` |
There was a problem hiding this comment.
Typo: "consdier" should be "consider".
| Why `2^n - 1` ? Let's consdier `n = 9` | |
| Why `2^n - 1` ? Let's consider `n = 9` |
| 0 1 1 1 1 1 1 1 1 1 | ||
| ``` | ||
|
|
||
| It can help us to filp the bit of `x`. For example, `x = -1` |
There was a problem hiding this comment.
Typo: "filp" should be "flip".
| It can help us to filp the bit of `x`. For example, `x = -1` | |
| It can help us to flip the bit of `x`. For example, `x = -1` |
| If `x` is a positive number, shift it. Otherwise, use `2^n - 1` to create an `n` one mask. | ||
| Then we can use the shift operator to achieve the same result as we use the division operator. | ||
|
|
||
| ## x86 Unsign Integer Division |
There was a problem hiding this comment.
Typo: "Unsign" should be "Unsigned" for consistency with the previous section "x86 Signed Integer Division".
| ## x86 Unsign Integer Division | |
| ## x86 Unsigned Integer Division |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new blog post with study notes on compiler optimizations for integer division. The notes are well-structured and cover both x86 and ARM architectures. I've found a few typos, grammatical errors, and some technical inaccuracies in the explanations. My review includes suggestions to correct these issues to improve the clarity and correctness of the post. The most critical ones are an incorrect explanation of the shrl instruction and an inaccurate diagram for two's complement addition.
| ``` | ||
| Position | 32 (Sign Bit) 10 1 | ||
| | v v v | ||
| Carry | 1 1 1 1 1 ... 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0 |
There was a problem hiding this comment.
The 'Carry' row in this diagram appears to be incorrect and could be confusing. For example, when adding the least significant bits (1 + 1), the result is 0 with a carry of 1. However, your diagram shows a carry of 0 for this position. It might be clearer to either correct the carry propagation or remove this row entirely to avoid misleading readers.
|
|
||
| Instruction: | ||
| ``` | ||
| shrl <imm>, <Rd> ; shr := Shift Right Logical, that is <Rd> = <Rd> << <imm> |
There was a problem hiding this comment.
The explanation for shrl is incorrect. It states that it's a left shift (<<), but shrl performs a logical right shift (>>). This is a critical error in the explanation that could confuse readers.
| shrl <imm>, <Rd> ; shr := Shift Right Logical, that is <Rd> = <Rd> << <imm> | |
| shrl <imm>, <Rd> ; shr := Shift Right Logical, that is <Rd> = <Rd> >> <imm> |
| To resolve this problem, the compiler adds `2^n - 1` to the negative number. | ||
| In this case, n = 9, hence it is `2^9 - 1 = 512 - 1 = 511 = 0x1FF` | ||
|
|
||
| Why `2^n - 1` ? Let's consdier `n = 9` |
There was a problem hiding this comment.
There's a typo here. 'consdier' should be 'consider'.
| Why `2^n - 1` ? Let's consdier `n = 9` | |
| Why `2^n - 1` ? Let's consider `n = 9` |
|
|
||
| Why `2^n - 1` ? Let's consdier `n = 9` | ||
|
|
||
| In binary, `2^9 - 1` creates the exactly 9 ones: |
There was a problem hiding this comment.
There's a minor grammatical error here. 'the exactly' should be 'exactly'.
| In binary, `2^9 - 1` creates the exactly 9 ones: | |
| In binary, `2^9 - 1` creates exactly 9 ones: |
| 0 1 1 1 1 1 1 1 1 1 | ||
| ``` | ||
|
|
||
| It can help us to filp the bit of `x`. For example, `x = -1` |
There was a problem hiding this comment.
There's a typo here ('filp' should be 'flip'). Additionally, the phrase 'flip the bit' might be misleading. This operation doesn't flip bits but adds a 'bias' to negative numbers to ensure the subsequent arithmetic right shift correctly truncates towards zero, matching C's division behavior.
| It can help us to filp the bit of `x`. For example, `x = -1` | |
| It can help us to flip the bit of `x`. For example, `x = -1` |
|
|
||
| Hence, the compiler does `(-1 + 511) / 512 = 510 / 512 = 510 >> 9 = 0`, we get the correct result. | ||
|
|
||
| In summary, the compiler use the `cmovens` and `test` instructions to detect whether `x` is positive. |
There was a problem hiding this comment.
There are a couple of small issues here: a typo in the instruction name (cmovens should be cmovnsl or cmovns), and a grammatical error ('use' should be 'uses').
| In summary, the compiler use the `cmovens` and `test` instructions to detect whether `x` is positive. | |
| In summary, the compiler uses the `cmovnsl` and `test` instructions to detect whether `x` is positive. |
| If `x` is a positive number, shift it. Otherwise, use `2^n - 1` to create an `n` one mask. | ||
| Then we can use the shift operator to achieve the same result as we use the division operator. | ||
|
|
||
| ## x86 Unsign Integer Division |
There was a problem hiding this comment.
There's a typo in the section title. 'Unsign' should be 'Unsigned'.
| ## x86 Unsign Integer Division | |
| ## x86 Unsigned Integer Division |
| Instructions: | ||
| ``` | ||
| - add <Wd>, <Wn>, #imm ; w8 = w0 + 0x1ff | ||
| - cmp <Wn>, #imm ; Compares w8 with #0x0, and update the processor flags NZCV |
There was a problem hiding this comment.
The comment for the cmp instruction is incorrect. It says it compares w8 with #0x0, but the instruction itself (cmp w0, #0x0) compares w0.
| - cmp <Wn>, #imm ; Compares w8 with #0x0, and update the processor flags NZCV | |
| - cmp <Wn>, #imm ; Compares w0 with #0x0, and update the processor flags NZCV |
| ``` | ||
|
|
||
| ## References | ||
| - https://developer.arm.com/documentation/ddi0601/2025-12/AArch64-Registers/NZCV--Condition-Flags |
There was a problem hiding this comment.
This link to the ARM documentation appears to be broken because of the future date 2025-12 in the URL, which causes a 404 error. You can fix this by replacing the date with latest to point to the most recent version of the documentation.
| - https://developer.arm.com/documentation/ddi0601/2025-12/AArch64-Registers/NZCV--Condition-Flags | |
| - https://developer.arm.com/documentation/ddi0601/latest/AArch64-Registers/NZCV--Condition-Flags |
1 participant
No description provided.