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

Inline functions in a referenced project optimized incorrectly #17903

Closed
rca22 opened this issue Oct 21, 2024 · 10 comments · Fixed by #18025
Closed

Inline functions in a referenced project optimized incorrectly #17903

rca22 opened this issue Oct 21, 2024 · 10 comments · Fixed by #18025
Assignees
Labels
Area-Compiler-Optimization The F# optimizer, release code gen etc. Bug help wanted Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code.
Milestone

Comments

@rca22
Copy link

rca22 commented Oct 21, 2024

In the attached example, we create three types;

  • ID, in ClassLibrary1
  • ID2, in ClassLibrary1, which relies on ID
  • ID3 in ConsoleApp1, which relies on ID2.

In each of these we override equality and comparison operators, using inline functions to improve performance for ID3. (Note that this repro is a minimal example from more complicated code where this makes more sense!)

We create instances of each in the console app. and check that two unequal values of each type are reported as unequal by the compiler.
In Release mode only, the third assertion, that two obviously different instances of ID3 are unequal fails.

To repro, please run the attached program.
repro.zip

Expected behavior

I expect the program to exit with code 0.

Actual behavior

An exception is thrown in the last equality check, for aBarBar versus bBarBar

Known workarounds

Running this in debug mode fixes the issue.
Putting the definitions of ID and ID2 in the same project as ID3 fixes the issue. Because of this, I suspect this issue is something to do with incorrect optimisations around inlining functions from other projects.
Changing ID from a struct into a class by removing all the attributes associated with it fixes the issue.

Related information

Tested in .NET 8, SDKs 8.0.100 and 8.0.403
Windows 10.
VS 2022 for editing.

@vzarytovskii
Copy link
Member

vzarytovskii commented Oct 21, 2024

An exception is thrown in the last equality check, for aBarBar versus bBarBar

What's the exception? What's yielded in the nightly 9.0 sdk? Does it change with realsig off? It could also be related to the additional equality generated.

@rca22
Copy link
Author

rca22 commented Oct 21, 2024

An exception is thrown in the last equality check, for aBarBar versus bBarBar

What's the exception? What's yielded in the nightly 9.0 sdk? Does it change with realsig off? It could also be related to the additional equality generated.

To be clear the last equality check reads if aBarBar = bBarBar then failwith "Expect aBarBar and bBarBar to differ"; earlier aBarBar and bBarBar are set up to be clearly different. The exception is not an internal one; just there to indicate the deviation from expected behaviour.

@vzarytovskii
Copy link
Member

An exception is thrown in the last equality check, for aBarBar versus bBarBar

What's the exception? What's yielded in the nightly 9.0 sdk? Does it change with realsig off? It could also be related to the additional equality generated.

To be clear the last equality check reads if aBarBar = bBarBar then failwith "Expect aBarBar and bBarBar to differ"; earlier aBarBar and bBarBar are set up to be clearly different. The exception is not an internal one; just there to indicate the deviation from expected behaviour.

I see, thanks. It appears that it's failing in SDK 6.0, could've been there forever.

@T-Gro
Copy link
Member

T-Gro commented Oct 21, 2024

This is what the decompiled Release code looks like for the Equals check of ID3.
As you can see, the name ID4 was being reused for both sides, ending up comparing a string with the same string, all the time.

Image

@vzarytovskii
Copy link
Member

This is what the decompiled Release code looks like for the Equals check of ID3.
As you can see, the name ID4 was being reused for both sides, ending up comparing a string with the same string, all the time.

Image

That's some awkward codegen. We will need to see whether it's a recent regression. If it is, we need to fix it for 9.0.200

@T-Gro T-Gro modified the milestones: Backlog, November-2024 Oct 29, 2024
@T-Gro T-Gro added Area-Compiler-Optimization The F# optimizer, release code gen etc. help wanted labels Nov 1, 2024
@vzarytovskii
Copy link
Member

vzarytovskii commented Nov 11, 2024

Update, the new minimal repro:

Library.fs (this file should be in a separate project):

namespace Library

#nowarn "346"

[<Struct>]
type ID (value : string) =
    member _.Value = value
    member inline this.Hello(other: ID) = System.Console.WriteLine(this.Value + " " + other.Value)

type ID2 = { Value : ID } with
    member inline this.Hello(other: ID2) = this.Value.Hello other.Value

Program.fs:

open Library

[<EntryPoint>]
let main _ =

    let aBar = { Value = ID "a" }
    let bBar = { Value = ID "b" }

    aBar.Hello(bBar)

    0

It is also not the case with static members. So the issue is that local seems not to be introduced for the this parameter of the struct member.

Something must be missing in the optimization data/environment, since this doesn't happen if any of the following are met:

  1. Library and program are in one project
  2. ID type is a non-struct
  3. ID2 type is a non-record

@abonie abonie added Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. and removed Needs-Triage labels Nov 11, 2024
@vzarytovskii
Copy link
Member

TLDR of the issue is that we reuse locals when we shouldn't be .

@vzarytovskii
Copy link
Member

So it seems that we don't mark copyOfStruct (copy of this) as used local. It seems it's in the wrong place in the TAST.

@vzarytovskii
Copy link
Member

So, the issue is that we leak address of this defensive copy out of its scope, one immediate solution would be is to never realloc for copyOfStruct (it's a special compiler name, we don't indicate it any other way):

showTerm: pass-end:
top implementation 
  Module Defs
    open ... 
    Module Defs
      module Program
        Module Defs
          open ... 
          let main [<.ctor>] ! <0>[1] =
                fun _arg1 ->
                  let aBar =
                        {Value=.ctor "a"}
                  let bBar =
                        {Value=.ctor "b"}
                  __debugPoint( /Users/u/code/issues/gh/17903/ConsoleApp1/Program.fs(9,4)-(9,20) )
                  let this =
                        let mutable copyOfStruct =
                              aBar.# Value
                        & copyOfStruct ()
                  let otherid =
                        bBar.# Value
                  System.Console.WriteLine [System.String.Concat [get_Value this ();" ";get_Value (& otherid ()) ()]]
                  __debugPoint( /Users/u/code/issues/gh/17903/ConsoleApp1/Program.fs(11,4)-(11,5) )
                  0

vzarytovskii added a commit to vzarytovskii/fsharp that referenced this issue Nov 18, 2024
@vzarytovskii
Copy link
Member

We shouldn't be generating it in general, but for now not reallocating it is fine:
Image

vzarytovskii added a commit to vzarytovskii/fsharp that referenced this issue Nov 27, 2024
vzarytovskii added a commit that referenced this issue Nov 29, 2024
Co-authored-by: vzarytovskii <1260985+vzarytovskii@users.noreply.github.com>
Co-authored-by: Petr Pokorny <petr@innit.cz>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compiler-Optimization The F# optimizer, release code gen etc. Bug help wanted Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants