Factored IRGenSILFunction::visitGraphOperationInst() to handle ops other than "Const". #19575
Conversation
…her than "Const". Added a new unit test based on "Add".
|
@swift-ci please test tensorflow |
mhong
changed the title
potential merge conflicts when we downstream code from the master branch.
…ored_graphop_irgen
| @@ -28,6 +28,8 @@ | |||
| #include "swift/AST/ParameterList.h" | |||
| #include "swift/AST/Pattern.h" | |||
| #include "swift/AST/SubstitutionMap.h" | |||
| // SWIFT_ENABLE_TENSORFLOW | |||
There was a problem hiding this comment.
Clarification question: Why do we have #ifdefs in some places and comment in other places?
There was a problem hiding this comment.
#ifdefs is intended to makes sure our branch can still be compiled without TF support. This is probably not consistently enforced though.
I'm also not sure if that's necessary. e.g. As a strategy to upstream our code, it might be sufficient to always include the TF related bits into the compiled binary, but protect the TF code path under some compiler flags. We can discuss this more outside this PR scope.
| @@ -1993,54 +1994,131 @@ void IRGenSILFunction::visitGraphOperationInst(GraphOperationInst *i) { | |||
| auto eagerContext = Builder.CreateCall(getContextFn, {}); | |||
|
|
|||
| // Create a TFE op as in: | |||
| // auto* op = TFE_NewOp(ctx, "Const", status); | |||
| // auto* op = TFE_NewOp(ctx, "OpName", status); | |||
There was a problem hiding this comment.
Doesn't the name have to be unique?
There was a problem hiding this comment.
In the eager API, the op name is an op type name like "Const", not the op node name (a concept that only exists in the graph mode).
|
Thanks for the review! |
The code structure resembles
TFGraphFunctionLowering::visitGraphOperationInst().Added a new unit test based on "Add".