[temp] Narrow VPTO changes in LLVM21 upgrade#825
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request upgrades the LLVM/MLIR dependency from LLVM 19 to LLVM 21, adapting the codebase to various API changes such as using memTy.getStridesAndOffset, applyPatternsGreedily, and updating EmitC variable generation to handle LValueType and LoadOp. It also updates build documentation, Dockerfiles, and Python dependencies. The review feedback highlights Python 3.8 compatibility issues in type annotations within test_docs_as_test.py, an O(N^2) complexity bottleneck in isPTOEntryFunction that can be optimized with an early-exit loop, and a potential crash in kernel.py due to an uncaught ValueError during file decoding.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| from dataclasses import dataclass | ||
| from pathlib import Path | ||
| from typing import Iterable | ||
| from typing import Iterable, Optional |
There was a problem hiding this comment.
Since the project supports Python 3.8+ (as documented in README.md), using standard collection types like dict and list as generics in type annotations (PEP 585) will cause a runtime TypeError on Python 3.8. Import Dict and List from typing to maintain compatibility.
| from typing import Iterable, Optional | |
| from typing import Dict, Iterable, List, Optional |
| compile_kwargs: dict[str, object] | None = None | ||
| fixture: str | None = None | ||
| symbol: Optional[str] = None | ||
| compile_kwargs: Optional[dict[str, object]] = None |
There was a problem hiding this comment.
Use Dict from typing instead of dict for Python 3.8 compatibility.
| compile_kwargs: Optional[dict[str, object]] = None | |
| compile_kwargs: Optional[Dict[str, object]] = None |
|
|
||
|
|
||
| def find_block_metadata(path: Path, lines: list[str], fence_line: int) -> DocBlockMetadata | None: | ||
| def find_block_metadata(path: Path, lines: list[str], fence_line: int) -> Optional[DocBlockMetadata]: |
There was a problem hiding this comment.
Use List from typing instead of list for Python 3.8 compatibility.
| def find_block_metadata(path: Path, lines: list[str], fence_line: int) -> Optional[DocBlockMetadata]: | |
| def find_block_metadata(path: Path, lines: List[str], fence_line: int) -> Optional[DocBlockMetadata]: |
| extra_namespace: Optional[dict[str, object]] = None, | ||
| ) -> dict[str, object]: |
There was a problem hiding this comment.
Use Dict from typing instead of dict for Python 3.8 compatibility.
| extra_namespace: Optional[dict[str, object]] = None, | |
| ) -> dict[str, object]: | |
| extra_namespace: Optional[Dict[str, object]] = None | |
| ) -> Dict[str, object]: |
| bool mlir::pto::isPTOEntryFunction(func::FuncOp func) { | ||
| if (!func || func.isDeclaration()) | ||
| return false; | ||
| return hasExplicitPTOEntryAttr(func); | ||
| if (auto attr = func->getAttrOfType<BoolAttr>(kEffectivePTOEntryAttrName)) | ||
| return attr.getValue(); | ||
| if (hasExplicitPTOEntryAttr(func)) | ||
| return true; | ||
|
|
||
| ModuleOp module = func->getParentOfType<ModuleOp>(); | ||
| if (!module) | ||
| return false; | ||
| SmallVector<func::FuncOp> defs = getPTOFunctionDefinitions(module); | ||
| return defs.size() == 1 && defs.front() == func; | ||
| } |
There was a problem hiding this comment.
Calling getPTOFunctionDefinitions inside isPTOEntryFunction performs a full isPTOEntryFunction is called in a loop over all functions (such as in validatePTOEntryFunctions), this results in false as soon as a second definition is encountered, reducing the average complexity to
bool mlir::pto::isPTOEntryFunction(func::FuncOp func) {
if (!func || func.isDeclaration())
return false;
if (auto attr = func->getAttrOfType<BoolAttr>(kEffectivePTOEntryAttrName))
return attr.getValue();
if (hasExplicitPTOEntryAttr(func))
return true;
ModuleOp module = func->getParentOfType<ModuleOp>();
if (!module)
return false;
func::FuncOp singleFunc = nullptr;
for (auto f : module.getOps<func::FuncOp>()) {
if (!f.isDeclaration()) {
if (singleFunc)
return false;
singleFunc = f;
}
}
return singleFunc == func;
}| except SyntaxError: | ||
| try: | ||
| module = ast.parse(Path(path).read_text(encoding="utf-8")) | ||
| except (OSError, IOError, SyntaxError): |
There was a problem hiding this comment.
If the file at path contains invalid UTF-8 bytes, Path(path).read_text(encoding="utf-8") will raise a UnicodeDecodeError (which inherits from ValueError). Since ValueError is not caught by except (OSError, IOError, SyntaxError), this will cause a compiler crash. Add ValueError to the caught exceptions to handle this gracefully.
| except (OSError, IOError, SyntaxError): | |
| except (OSError, IOError, SyntaxError, ValueError): |
Codex Review该评论由 review 机器人自动更新。
SummaryReview failed at stage Findings未生成结构化 findings，因为 review 过程提前失败。 Log Tail |
mouliangyu
force-pushed
the
codex/vpto-llvm21-api-only-temp
branch
9 times, most recently
from
fec267a to
0ca5522
Compare
mouliangyu
force-pushed
the
codex/vpto-llvm21-api-only-temp
branch
from
0ca5522 to
c9a83ca
Compare
3 participants
Temporary PR for reviewing the LLVM21 upgrade shape relative to main.
Focus:
Validation run locally: