Expand LinxISA toolchain coverage for FP block headers and hidden operands#23
Expand LinxISA toolchain coverage for FP block headers and hidden operands#23zhoubot wants to merge 1 commit into
Conversation
…rands Assembler, blockify, MC backend, and tests are updated together so the current ISA surface round-trips the forms that the documentation and runtime work now depend on. The packaged changes also cover large SSR IDs, HL immediate spellings, prefetch round-trips, and FP-aware BSTART header selection. Constraint: Package the existing model-generated delta without rebasing the detached submodule checkout first Rejected: Split parser, backend, and tests into separate commits | user requested direct packaging of the current worktree state Confidence: medium Scope-risk: moderate Directive: Rebuild clang as well as llvm-mc when touching assembler or MC paths because AVS .S flows use the integrated assembler Tested: git diff --check Not-tested: llvm build, MC test suite Co-authored-by: OmX <omx@oh-my-codex.dev>
![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 introduces floating-point block headers (BSTART_FP variants) and associated logic to detect if blocks require FP headers in the LinxISA target. It also refactors vector pipe destination code assignment, updates relocation handling for RE_LINX_PC_INDIRECT in the ELF linker, and integrates ScalarEvolution into the SIMT auto-vectorizer. The review feedback highlights two critical issues: first, a potential silent miscompilation in InputSection.cpp when getLinxPCRelHi20 returns null, which can be resolved by directly returning the symbol's virtual address; second, a logic bug in LinxISABlockify.cpp where NextIndex is incremented but never encoded into the destination code, ignoring the queue index.
| if (const Relocation *hiRel = getLinxPCRelHi20(ctx, this, r)) | ||
| return getRelocTargetVA(ctx, *hiRel, getVA(hiRel->offset)); | ||
| // For Linx ADDTPC+ADDI/ADDIW materialization, the HI20 relocation only | ||
| // establishes the page-relative anchor. The paired LO12 relocation must | ||
| // receive the final symbol VA's low 12 bits, not the HI20 page delta. | ||
| return r.sym->getVA(ctx, a); | ||
| return 0; |
There was a problem hiding this comment.
If getLinxPCRelHi20 returns nullptr (which now happens when hiSec is null, as allowed by the changes in getLinxPCRelHi20), the code falls through and returns 0. This silently resolves the relocation to 0 instead of the actual symbol's virtual address (r.sym->getVA(ctx, a)), leading to a silent miscompilation/link error.
Since the LO12 relocation's value only needs the low 12 bits of the target symbol's VA, we should return r.sym->getVA(ctx, a) directly, while still calling getLinxPCRelHi20 for validation and warnings.
(void)getLinxPCRelHi20(ctx, this, r);
// For Linx ADDTPC+ADDI/ADDIW materialization, the HI20 relocation only
// establishes the page-relative anchor. The paired LO12 relocation must
// receive the final symbol VA's low 12 bits, not the HI20 page delta.
return r.sym->getVA(ctx, a);| auto nextCode = [&](unsigned Class, unsigned &NextIndex) { | ||
| return (Class << 5) | (NextIndex++ & 0x1fu); | ||
| unsigned Code = 0; | ||
| switch (Class) { | ||
| case 4: | ||
| Code = 0u; // DST_FVEC_VT | ||
| break; | ||
| case 5: | ||
| Code = 1u; // DST_FVEC_VU | ||
| break; | ||
| case 6: | ||
| Code = 2u; // DST_FVEC_VM | ||
| break; | ||
| case 7: | ||
| Code = 3u; // DST_FVEC_VN | ||
| break; | ||
| default: | ||
| llvm_unreachable("unexpected vector head queue class"); | ||
| } | ||
| ++NextIndex; | ||
| return Code; | ||
| }; |
There was a problem hiding this comment.
In nextCode, NextIndex is incremented (++NextIndex), but its value is never used to construct Code. This means the queue index is not encoded in the returned destination code, which will cause all destinations of the same class to share the same code (0, 1, 2, or 3), ignoring the queue index. Since kMaxIndex is still 8 (as seen in the change to LinxISASIMTAutoVectorize.cpp), the index must be encoded. Please ensure NextIndex is correctly integrated into Code (e.g., by shifting and ORing it into the appropriate bits).
1 participant
Summary
Testing
git diff --checkNot tested