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

[META][AutoDiff] Tech debt from tests #77773

Open
2 of 20 tasks
kovdan01 opened this issue Nov 21, 2024 · 8 comments
Open
2 of 20 tasks

[META][AutoDiff] Tech debt from tests #77773

kovdan01 opened this issue Nov 21, 2024 · 8 comments
Labels
AutoDiff compiler The Swift compiler itself task ☂️ umbrella issue Flag: An umbrella issue

Comments

@kovdan01
Copy link
Contributor

kovdan01 commented Nov 21, 2024

Description

I've assigned preliminary priorities for the issues, but this should be revisited.

High priority:

Medium priority:

Low priority:

  • [AutoDiff][test] Investigate if a BooleanLiteralFolding-related crash was fixed #77762
    Notes:

    1. TODO: cannot use literal true because it crashes
    2. TODO: cannot use literal false because it crashes

    Tests:

    1. test/AutoDiff/compiler_crashers_fixed/58660-conflicting-debug-info-inlining.swift
    2. test/AutoDiff/compiler_crashers_fixed/issue-58123-mutating-functions-with-control-flow.swift
    3. test/AutoDiff/validation-test/address_only_tangentvector.swift
    4. test/AutoDiff/validation-test/control_flow.swift
  • [AutoDiff][test] Ensure if @differentiable function SILGen thunking is fixed #77775
    Note: TODO(TF-851): Uncomment the tests below after @differentiable function SILGen thunking is fixed.
    Test: test/AutoDiff/SILOptimizer/generics.swift

  • Note: TODO: Re-enable the boolean-literal-folding pass and fix the test accordingly
    Test: test/AutoDiff/SILOptimizer/activity_analysis.swift

  • Note: TODO(TF-956): Improve location of active enum non-differentiability errors
    Test: test/AutoDiff/SILOptimizer/differentiation_control_flow_diagnostics.swift

  • Note: TODO(TF-957): Improve non-differentiability errors for for-in loops
    Test: test/AutoDiff/SILOptimizer/differentiation_control_flow_diagnostics.swift

  • Note: TODO(TF-788): Re-enable non-varied result warning.
    Test: test/AutoDiff/SILOptimizer/differentiation_diagnostics.swift

  • Note: TODO(TF-482): Remove diagnostics when @differentiable attributes are also uniqued based on generic requirements.
    Test: test/AutoDiff/Sema/differentiable_attr_type_checking.swift

  • Note: TODO(TF-650): It would be nice to diagnose protocol default implementation with missing @differentiable attribute.
    Test: test/AutoDiff/Sema/differentiable_attr_type_checking.swift

  • Note: TODO(TF-632): Fix "'TangentVector' is not a member type of 'Self'" diagnostic. The underlying error should appear instead: "covariant 'Self' can only appear at the top level of method result type".
    Test: test/AutoDiff/Sema/differentiable_attr_type_checking.swift

  • Note: FIXME: Enable test for all platforms after debugging ([SR-12741] TEST 'Swift(iphonesimulator-i386) :: AutoDiff/validation-test/control_flow.swift #55186).
    Test: test/AutoDiff/validation-test/control_flow.swift

  • Note: TODO(TF-1065): Consider disallowing qualified operators / operator names
    Tests:

    1. test/AutoDiff/Parse/transpose_attr_parse.swift
    2. test/AutoDiff/Sema/transpose_attr_type_checking.swift

Existing issues which require discussion about priorities, semantics, etc:

Class-related:

  1. [META][AutoDiff] Fix autodiff for classes and class-valued arguments #65012 and its sub-issues.
  2. [SR-12148] Make differentiation work with class types #52130 and its sub-issues.

Forward-mode-related:

  1. [SR-13464] Missing support for classes in forward-mode AD #55906
  2. [SR-13210] Forward mode differentiation SIL verification crash #55650
  3. [SR-13250] AutoDiff test failure: test/AutoDiff/validation-test/differentiable_protocol_requirements.swift #55690
    Note: FIXME: Disabled due to test failure with -O
    Test: test/AutoDiff/validation-test/differentiable_protocol_requirements.swift

Entire tests which require discussion whether we want to address tech debt for them:

Class-related:

  1. test/AutoDiff/SILGen/vtable.swift
  2. test/AutoDiff/Sema/DerivedConformances/class_differentiable.swift
  3. test/AutoDiff/validation-test/class_differentiation.swift

Forward-mode-related:

  1. test/AutoDiff/SILOptimizer/forward_mode_diagnostics.swift
  2. test/AutoDiff/validation-test/forward_mode_simd.swift
  3. test/AutoDiff/validation-test/forward_mode_simple.swift

Marked as TODO/FIXME/etc but require discussion whether we want to address them:

Class-related:

  1. Note: FIXME(rdar://74380324)
    Test: test/AutoDiff/TBD/derivative_symbols.swift
    FIXMEs are related to initializer and other special functions differentiation support
  2. Note: FIXME(TF-1080): Fix incorrect class property modify accessor derivative values.
    Test: test/AutoDiff/validation-test/inout_parameters.swift
  3. Note: TODO(TF-649): Enable @derivative to override derivatives for original declaration defined in superclass.
    Test: test/AutoDiff/Sema/derivative_attr_type_checking.swift
  4. Note: FIXME: Enable derivative registration for class property/subscript setters ([SR-13096] Fix autodiff typing rules for class-typed function parameters #55542).
    Test: test/AutoDiff/Sema/derivative_attr_type_checking.swift
    This looks working for structs.
  5. Note: FIXME(rdar://74380324)
    Test: test/AutoDiff/TBD/derivative_symbols.swift
    The FIXME is related to the following crash:
    1.      Swift version 6.1-dev (LLVM 0f86f354a7bc883, Swift e6b4e0f9f171a51)
    2.      Compiling with effective version 4.1.50
    3.      While evaluating request ASTLoweringRequest(Lowering AST to SIL for module test)
    6.      While verifying SIL function "@$s4test5ClassCyACSfcfCTJVfSUpSr".test/AutoDiff/TBD/derivative_symbols.swift
    
  6. Note: FIXME(TF-1175): Class operands should always be marked active.
    Test: test/AutoDiff/validation-test/property_wrappers.swift
  7. Note: FIXME(TF-1176): Some values are incorrectly not marked as active: %16, etc.
    Test: test/AutoDiff/SILOptimizer/activity_analysis.swift
    This is related to class property modify accessor.

Forward-mode-related:

  1. Note: TODO(TF-1254): Support forward-mode differentiation and test generated differentials.
    Test: test/AutoDiff/SILOptimizer/semantic_member_accessors_sil.swift
  2. Note: TODO(TF-1173): Move floating-point mutating operation tests to test/AutoDiff/stdlib/floating_point.swift.gyb when forward-mode differentiation supports inout parameter differentiation.
    Test: test/AutoDiff/validation-test/inout_parameters.swift
  3. Note: TODO(TF-1254): Support and test forward-mode differentiation.
    Test: test/AutoDiff/validation-test/property_wrappers.swift
  4. Note: FIXME(TF-1033): Fix forward-mode ownership error for tuple with non-active
    Test: test/AutoDiff/validation-test/simple_math.swift

Linear functions related:

  1. Note: TODO(TF-900, TF-902): Uncomment the following line to test loading a linear function from memory and direct calls to a linear function.
    Test: test/AutoDiff/SILGen/differentiable_function.swift
  2. Note: TODO: Add tests for @differentiable(_linear) functions.
    Test: test/AutoDiff/IRGen/loadable_by_address.swift

Additional information

No response

@kovdan01 kovdan01 added task triage needed This issue needs more specific labels labels Nov 21, 2024
@kovdan01
Copy link
Contributor Author

Tagging @asl

@asl
Copy link
Contributor

asl commented Nov 21, 2024

@JaapWijnen @rxwei

@JaapWijnen
Copy link
Contributor

This is great work! Thanks @kovdan01

Priority wise, to me the high priority ones look good to me.
Medium priority, the most important ones to me are:

  • Note: TODO(TF-1030): This will eventually not be an error. (which you've already provided a fix for)
  • Note: TODO(TF-982): Lift this restriction and add proper support. (This would be huge but seems like a larger effort to get to the finish line? Maybe I'm mistaken though)
  • Note: TODO: Support modify accessors (Same here but this is a large effort and luckily already ongoing!)

Regarding the existing issues priority:
I personally feel we should not prioritise class differentiation and forward differentiation. In my opinion we should focus on getting reverse mode differentiation on value types in a really good place first.
I would like to discuss adding more general support for enums however instead of the current "specialised" support for the Optional type.
I'll make sure to do some research and do a write up of what I think differentiating through enums/sum-types should look like as a general solution.

Hope this helps

@JaapWijnen
Copy link
Contributor

Oh also, the TF related links don't seem to work and redirect to the following page: https://github.com/apple/swift-issues/issues

Is this a recent change that these issues are no longer hosted there? Not sure I haven't looked at the page for a while.

@kovdan01
Copy link
Contributor Author

kovdan01 commented Nov 27, 2024

Oh also, the TF related links don't seem to work and redirect to the following page: https://github.com/apple/swift-issues/

Is this a recent change that these issues are no longer hosted there? Not sure I haven't looked at the page for a while.

It looks like they are not hosted there for at least several months. As far as I understood, they are just gone and were not transferred to github properly (some of them were, but not all of them), unfortunately.

It looks like that swift github repository still has some rules so TF-XXX text in issues is automatically converted to corresponding dangling link - it might be a bit confusing.

@asl Please correct me if I'm mistaken

@asl
Copy link
Contributor

asl commented Nov 27, 2024

TF issues from TensorFlow Jira were never converted to github issues. So, they are gone forever.

@kovdan01
Copy link
Contributor Author

@JaapWijnen Thanks for assigning priorities! One minor comment regarding the following: it looks like that the observable error is the same but root cause is different (not related to curry thunks). So it looks like it would need a separate patch

  • Note: TODO(TF-1030): This will eventually not be an error. (which you've already provided a fix for)

@AnthonyLatsis AnthonyLatsis added AutoDiff ☂️ umbrella issue Flag: An umbrella issue compiler The Swift compiler itself and removed triage needed This issue needs more specific labels labels Nov 27, 2024
@asl
Copy link
Contributor

asl commented Jan 8, 2025

This one looks worth checking as well:
AutoDiff/validation-test/control_flow.swift:

// rdar://71642726 this test is crashing with optimizations.
// REQUIRES: swift_test_mode_optimize_none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoDiff compiler The Swift compiler itself task ☂️ umbrella issue Flag: An umbrella issue
Projects
None yet
Development

No branches or pull requests

4 participants