[AutoDiff] Be able to parse linear argument in @differentiating attribute
#25257
Conversation
- Adds a 'linear` argument to '@differentiable' attribute - 'linear' argument is optional, but when added, needs to be the first argument in the attribute
| @@ -829,12 +829,12 @@ ParserResult<DifferentiableAttr> | |||
| Parser::parseDifferentiableAttribute(SourceLoc atLoc, SourceLoc loc) { | |||
| StringRef AttrName = "differentiable"; | |||
| SourceLoc lParenLoc = loc, rParenLoc = loc; | |||
|
|
|||
|
|
|||
| bool linear; | |||
There was a problem hiding this comment.
Small update I wanted to do from last PR. Since linear is first argument in @differentiable, I also wanted it to be the first one declared.
| @@ -1504,6 +1504,8 @@ ERROR(attr_differentiable_expected_label,none, | |||
| // differentiating | |||
| ERROR(attr_differentiating_expected_original_name,PointsToFirstBadToken, | |||
| "expected an original function name", ()) | |||
| ERROR(attr_differentiating_expected_linear_wrt,none, | |||
| "expected either 'linear' or 'wrt:'", ()) | |||
There was a problem hiding this comment.
| "expected either 'linear' or 'wrt:'", ()) | |
| ERROR(attr_differentiating_expected_label_linear_or_wrt,none, | |
| "expected either 'linear' or 'wrt:'", ()) |
There was a problem hiding this comment.
I don't believe this suggestion was applied before the code was merged.
There was a problem hiding this comment.
Oh I thought this was a name change of the variable. Before it was attr_differentiating_expected_linear_wrt but now I changed it to attr_differentiating_expected_label_linear_or_wrt
There was a problem hiding this comment.
It's also about the indentation of the second line.
There was a problem hiding this comment.
Oops will fix this in the new PR misinterpreted the suggestion
|
|
||
| // expected-error @+2 {{expected ')' in 'differentiating' attribute}} | ||
| // expected-error @+1 {{expected declaration}} | ||
| @differentiating(foo, wrt: x,) |
There was a problem hiding this comment.
Could you please add a @differentiating(foo, wrt: x, linear) test?
There was a problem hiding this comment.
Ahh yes thanks for catching this test case! Will add it tomorrow morning 😴😄
|
@swift-ci please test tensorflow |
bartchr808
changed the title
linear argument in @differentiating attributelinear argument in @differentiating attribute
This PR is following this one about
@differentiable(linear).One thing to note in my tests is how I consider the
linearargument.@differentiating(linear)should be allowed as a user may create a function calledlinear, so there are 2 scenarios a user can be in:@differentiating(linear)-> Differentiating a function calledlinear.@differentiating(linear, linear)-> Differentiating a linear function calledlinearAlso, we previously weren't handling the case of trailing commas in the
@differentiatingattribute, so I made sure we were this time and also added corresponding tests.