Repository navigation
Conversation
…lePaddle#79749) ConvertPirAttribute2RuntimeAttribute rejected pir::ArrayAttribute values whose elements were FloatAttribute, Int32Attribute or Int64Attribute when the op schema declared pir::ArrayAttribute<pir::DoubleAttribute>. The values are losslessly promotable to double, so the guard produced a false negative and aborted inference with: ConvertPirAttribute2RuntimeAttribute not support [pir::ArrayAttribute<pir::DoubleAttribute>] This breaks PaddleX PP-StructureV3 (layout detection, table and formula recognition) on PaddlePaddle 3.x. Accept DoubleAttribute directly and promote Float/Int32/Int64; keep throwing for genuinely unsupported element types. Ref PaddlePaddle#79749 Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
|
|
Paddle-Bot Review Board （review完成)
可维护性建议（总分 88.5 / 100，只给建议，不影响评审结论）
各维度得分（仅评本 PR 改动，点击展开）
Powered by Nyanpasu claude with Opus 5.5 默认推理级别, please check the suggestions carefully. |
There was a problem hiding this comment.
🟡 Changes recommended
Kernel attribute arrays beginning with Float, Int32, or Int64 still throw in BuildPhiContext before reaching the converter because the shared double-array path requires a DoubleAttribute first element.
1 open issue
What changed in this PR
This PR broadens oneDNN’s PIR-to-runtime double-array conversion to address numeric attribute failures reported in #79749 and #77340.
Changes:
- Validates each array element rather than only the first.
- Converts Float, Int32, and Int64 attributes to
double, while retaining Double support. - Preserves
Unimplementederrors for unsupported element types.
| File | Description |
|---|---|
paddle/fluid/framework/new_executor/instruction/onednn/onednn_instruction.cc |
Adds per-element numeric conversion for double-array attributes. |
Review effort: Balanced
| } else if (array_list[i].isa<pir::FloatAttribute>()) { | ||
| vec_res.push_back( | ||
| static_cast<double>( | ||
| array_list[i].dyn_cast<pir::FloatAttribute>().data())); |
| if (array_list[i].isa<pir::DoubleAttribute>()) { | ||
| vec_res.push_back( | ||
| array_list[i].dyn_cast<pir::DoubleAttribute>().data()); | ||
| } else if (array_list[i].isa<pir::FloatAttribute>()) { |
There was a problem hiding this comment.
OneDNNPhiKernelInstruction 构造函数先在 L283/L305 调用 BuildPhiContext，它遍历的是同一组 AttrParams(true) 属性；而 pir_adaptor_util.h:378-390 中 pir::ArrayAttribute<pir::DoubleAttribute> 分支仍要求首元素是 DoubleAttribute，否则抛 attr type not support。本函数要到 L396 才转换 ctx_attr_，并且只在 op 带 extra_args 时执行。
结果有两种：
- 首元素为 Float/Int32/Int64：在
BuildPhiContext就失败，不会走到这里； - 首元素为 Double、后续元素不是：
BuildPhiContext会对这些元素直接dyn_cast<pir::DoubleAttribute>().data()。
建议把数值提升抽成一个共享 helper，BuildPhiContext 和这里都调用它。另请补充触发问题的具体 op 和属性名，确认修复后的路径确实生效。
| if (array_list.size() > 0) { | ||
| if (array_list[0].isa<pir::DoubleAttribute>()) { | ||
| for (size_t i = 0; i < array_list.size(); ++i) { | ||
| for (size_t i = 0; i < array_list.size(); ++i) { |
There was a problem hiding this comment.
pir::ArrayAttribute<pir::DoubleAttribute> 的接受规则，但 PR 没有回归测试。建议补充：
- 正向用例：double 数组属性中混合 Double/Float/Int32/Int64 元素，断言转换后的值和顺序正确；
- 负向用例：包含不支持的元素类型（如
StrAttribute），断言抛出Unimplemented。
该函数是 static，不方便直接单测，可以执行一个带 double[] 属性的 oneDNN 算子来覆盖（参考 test/cpp/new_executor/ 或现有 oneDNN PIR 测试），同时覆盖上一条提到的 BuildPhiContext 路径。
| array_list[i].dyn_cast<pir::Int64Attribute>().data())); | ||
| } else { | ||
| PADDLE_THROW(common::errors::Unimplemented( | ||
| "ConvertPirAttribute2RuntimeAttribute not support [%s] ", |
There was a problem hiding this comment.
💡 可维护性建议（错误处理一致性，不阻塞合入）：进入该分支时 attr_type_name 恒为 pir::ArrayAttribute<pir::DoubleAttribute>，而这个类型本身是支持的，报“不支持该类型”与实际原因不符。建议在消息中带上属性名和出错元素下标，例如 "ConvertPirAttribute2RuntimeAttribute: element %d of attr [%s] (%s) must be Double/Float/Int32/Int64"，参数为 i, attr_name, attr_type_name。
| @@ -106,15 +106,27 @@ static phi::Attribute ConvertPirAttribute2RuntimeAttribute( | |||
| auto array_list = attr.dyn_cast<pir::ArrayAttribute>().AsVector(); | |||
| std::vector<double> vec_res; | |||
| if (array_list.size() > 0) { | |||
There was a problem hiding this comment.
💡 可维护性建议（逻辑清晰度，不阻塞合入）：改成逐元素循环后，外层 if (array_list.size() > 0) 已不再保护任何 array_list[0] 访问。可以删除这一层判断及 L131 对应的 }，减少一级嵌套。
Problem
ConvertPirAttribute2RuntimeAttributethrows when an op schema declarespir::ArrayAttribute<pir::DoubleAttribute>but the concrete elements areFloatAttribute,Int32AttributeorInt64Attribute:The double-array branch validates only the first element and only accepts a
literal
pir::DoubleAttribute, then raises. The values are losslesslypromotable to
double, so the guard is a false negative.This breaks PaddleX
PP-StructureV3(layout detection → table/formularecognition) on PaddlePaddle 3.x. On
3.0.0b1the same pipeline aborts evenearlier with a
SIGSEGVinAnalysisPredictor::PrepareProgram.Fixes #79749 (related: #77340).
Change
DoubleAttributedirectly.FloatAttribute,Int32Attribute,Int64Attributetodouble.Unimplementedfor genuinely unsupported element types.No behavior change for well-typed double arrays; only previously-rejected
numeric inputs now succeed.
Verification
Built PaddlePaddle from this branch (CPU-only,
-DWITH_GPU=OFF; the GPU buildforces FlashAttention and fails compiling
flash_attn_with_bias_and_mask.cu)and ran the previously crashing pipeline:
PPStructureV3constructs and returns results (no exception, no segfault).colspanon a realeconomic-offer page.
R_{n}=F_{y}A_{g},\phi M_{n}=\phi F_{y}Z_{x}\geq M_{u}.Happy to add a C++ unit test for the attribute conversion if you point me to the
preferred location (
test/cpp/fluid/framework/new_executor/…?).