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

JIT Optimization: Merge multiple inc into one add #65025

Closed
deeprobin opened this issue Feb 8, 2022 · 8 comments · Fixed by #79346
Closed

JIT Optimization: Merge multiple inc into one add #65025

deeprobin opened this issue Feb 8, 2022 · 8 comments · Fixed by #79346
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@deeprobin
Copy link
Contributor

https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBLANgHwAEAmARgFgAoQgZgAIS6BhOgbyrs4fuwDsM6AQQAUfAQEMAlGw5c54gNQKA3LLmdFKtesIB2OuNWU5AX23badMXQBCo/gentj6jXQUBeOsSOvOeg18uM0oTIA

The codegen of B is better (less cpu cycles).

    public int A(int a) {
        a++; // inc edx
        a++; // inc edx
        return a;
    }
    
    public int B(int a) {
        a += 2; // add edx, 2
        return a;
    }

Expected result

- inc edx
- inc edx
+ add edx, 2
  mov eax, edx
  ret
@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Feb 8, 2022
@ghost
Copy link

ghost commented Feb 8, 2022

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBLANgHwAEAmARgFgAoQgZgAIS6BhOgbyrs4fuwDsM6AQQAUfAQEMAlGw5c54gNQKA3LLmdFKtesIB2OuNWU5AX23badMXQBCo/gentj6jXQUBeOsSOvOeg18uM0oTIA

The codegen of B is better (less cpu cycles).

    public int A(int a) {
        a++; // inc edx
        a++; // inc edx
        return a;
    }
    
    public int B(int a) {
        a += 2; // add edx, 2
        return a;
    }

Expected result

- inc edx
- inc edx
+ add edx, 2
  mov eax, edx
  ret

<table>
  <tr>
    <th align="left">Author:</th>
    <td>deeprobin</td>
  </tr>
  <tr>
    <th align="left">Assignees:</th>
    <td>-</td>
  </tr>
  <tr>
    <th align="left">Labels:</th>
    <td>

`area-CodeGen-coreclr`, `untriaged`

</td>
  </tr>
  <tr>
    <th align="left">Milestone:</th>
    <td>-</td>
  </tr>
</table>
</details>

@danmoseley
Copy link
Member

This does not seem to be what C++ compilers do: https://godbolt.org/z/r1f7Pxdfa
Curious to know why.

@omariom
Copy link
Contributor

omariom commented Feb 9, 2022

@danmoseley
With optimization flags https://godbolt.org/z/MPcor1xEP

@danmoseley
Copy link
Member

@omariom thanks, I thought that was default.

@deeprobin
Copy link
Contributor Author

@dubiousconst282
Copy link
Contributor

IMO this should be generalized to all operators.

Interestingly, copying to temporary variables will result in optimal code, so maybe this is related to SSA/constant propagation:

public int M(int x) {
    x *= 7;
    x *= 7;
    return x;
}
public int M2(int x) {
    int t1 = x * 7;
    int t2 = t1 * 7;
    return t2;
}
C.M(Int32)
    L0000: imul edx, 7
    L0003: imul edx, 7
    L0006: mov eax, edx
    L0008: ret

C.M2(Int32)
    L0000: imul eax, edx, 0x31
    L0003: ret

Sharplab

@JulieLeeMSFT
Copy link
Member

cc @dotnet/jit-contrib

@JulieLeeMSFT JulieLeeMSFT removed the untriaged New issue has not been triaged by the area owner label Feb 9, 2022
@JulieLeeMSFT JulieLeeMSFT added this to the Future milestone Feb 9, 2022
@AndyAyersMS
Copy link
Member

Note forward sub (#63720) does not handle cases where a local has multiple definitions and uses, even if each definition has just one use. It might not be too hard to extend it to cover these cases.

Eg if we run it again once SSA is built and track the number of uses of each SSA def.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 11, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 11, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
6 participants