Improve -O0 support, as progress towards fixing SR-8395: #18368
Conversation
Enhance constant expressions to handle copy_addr in top level context. Enhance deabstraction to constantfold scalarToTensor into a Const node. There is still more to do, all of the warnings in the testcase are bogus.
| @swift-ci please test tensorflow |
| // Dig the element type out of the TensorHandle result type. | ||
| auto eltType = | ||
| getTensorHandleElementType(inst->getType().getASTType()); | ||
| auto int32Ty = |
There was a problem hiding this comment.
add a comment that this is for shape?
or, given SymbolicValue::getArray({}, int32Ty, allocator) is used in a few places, consider making it a util method. the method addScalarShapeArrayAttr() in TFPartition.cpp uses such code as well.
| auto shapeId = SILTensorOpInfo::OperandClass::Shape; | ||
| auto shapeSuffix = SILTensorOpInfo::getOperandClassSuffix(shapeId); | ||
| attributes.push_back({ | ||
| context.getIdentifier(std::string("value") + shapeSuffix), shape |
There was a problem hiding this comment.
given the prev attr is called value$tensor, do we want to call this something like __shape$shape (i'm suggesting __ because this attr is a "pseudo attr" -- it won't be lowered to an op attr in the GraphDef)?
Also, do we still need to add things like $tensor, $shape at this point? IIUC, we'd like to delete TF graph lowering code that relies on such specifiers.
There was a problem hiding this comment.
Yeah, this is a direction I'd love to see! We'll need some of the specifiers, but not nearly all the ones we have today.
| } | ||
| } | ||
| | ||
| if (auto *cai = dyn_cast<CopyAddrInst>(user)) { |
There was a problem hiding this comment.
sorry I lost track of whether this chunk of code helps with the test case at hand.
I noticed 4 CopyAddr insts in the output of deabstraction pass, and they seem to be still present in the input to partition pass (when -O is on). Also they are not on tensor values. one example is:
copy_addr %147 to [initialization] %152 : $*Int64 // id: %153 (If this chunk helps with other test cases, including it in the patch is fine.)
There was a problem hiding this comment.
This is necessary to see through to the constants that get folded.
| allocator), | ||
| inst->getType(), inst->getLoc(), | ||
| DeviceType::ALL, B); | ||
| inst->replaceAllUsesWith(constant->getResult(0)); |
There was a problem hiding this comment.
This comment is unrelated to this PR, but about our constant folding implementation. Replacing the use of inst with a constant can trigger more constant folding opportunities. Will that be taken care of?
There was a problem hiding this comment.
RAUW won't do that, it just replaces the instruction itself. Remember that in this case we're producing a Const node, not something the generic SIL optimizer knows about. It doesn't handle tensorflow level constant folding.
…nsor's to invalid Const nodes.
| @swift-ci please test tensorflow |
1 similar comment
| @swift-ci please test tensorflow |
Enhance constant expressions to handle copy_addr in top level context.
Enhance deabstraction to constantfold scalarToTensor into a Const node.
There is still more to do, all of the warnings in the testcase are bogus.