amd64: support klauspost/compress amd64 ops#15
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15 +/- ##
==========================================
+ Coverage 69.55% 69.58% +0.02%
==========================================
Files 57 57
Lines 16431 16871 +440
==========================================
+ Hits 11429 11740 +311
- Misses 3685 3777 +92
- Partials 1317 1354 +37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 implements support for the SETCS (Set if Carry) instruction in the AMD64 backend. The changes include updating the arithmetic lowering logic to handle SETCS by loading the carry flag and adding a unit test to verify the implementation. A review comment suggests further improving instruction coverage by adding support for SETCC and SETNE, ensuring consistency with other conditional operations.
| return true, false, fmt.Errorf("amd64: unsupported bit op %s", op) | ||
|
|
||
| case "SETEQ", "SETGT", "SETHI": | ||
| case "SETEQ", "SETGT", "SETHI", "SETCS": |
There was a problem hiding this comment.
To improve arithmetic coverage and maintain consistency with the CMOVcc implementation (line 830), consider also adding support for SETCC (Carry Clear) and SETNE (Not Equal). These are common counterparts to SETCS and SETEQ which are already present in this block.
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
The core implementation is correct and consistent with the existing pattern — SETCS is properly dispatched in both the outer case list and the inner flag-loading switch, and loadFlag(c.flagsCFSlot) is the right helper (same as CMOVQCS). A few test quality and minor style issues below.
| mustLower("SETEQ", Instr{Raw: "SETEQ AX", Args: []Operand{{Kind: OpReg, Reg: AX}}}) | ||
| mustLower("SETGT", Instr{Raw: "SETGT ret+8(FP)", Args: []Operand{{Kind: OpFP, FPOffset: 8}}}) | ||
| mustLower("SETHI", Instr{Raw: "SETHI BX", Args: []Operand{{Kind: OpReg, Reg: BX}}}) | ||
| mustLower("SETCS", Instr{Raw: "SETCS AX", Args: []Operand{{Kind: OpReg, Reg: AX}}}) |
There was a problem hiding this comment.
Only the register-destination path is covered for SETCS. SETGT on the line above exercises both OpReg and OpFP ("SETGT ret+8(FP)"). Consider adding:
mustLower("SETCS", Instr{Raw: "SETCS ret+8(FP)", Args: []Operand{{Kind: OpFP, FPOffset: 8}}})to keep the FP-result-slot path tested for this instruction too.
| if err := c.storeReg(AX, "0"); err != nil { | ||
| t.Fatalf("storeReg(AX) error = %v", err) | ||
| } |
There was a problem hiding this comment.
storeReg(AX, "0") is a no-op here. Because Func{} is passed with no instructions, emitEntryAllocas never allocates a slot for AX, so storeReg returns nil without emitting any IR. The error check is dead and the call implies a precondition being set that isn't. Either remove it or structure the fixture so AX actually has an allocated slot.
| for _, want := range []string{ | ||
| "store i1 true, ptr %flags_cf", | ||
| "load i1, ptr %flags_cf", | ||
| "select i1 %", |
There was a problem hiding this comment.
"select i1 %" matches any select i1 % in the whole builder output and doesn't verify that the selected condition came from %flags_cf. A stronger assertion (e.g. checking that the load temporary is the one used in the select) would make the test actually validate the carry-flag wiring rather than just that a select appeared somewhere.
| case "SETCS": | ||
| cond = c.loadFlag(c.flagsCFSlot) |
There was a problem hiding this comment.
The sibling cases (SETGT, SETHI) have inline comments explaining what flag they check and the condition it represents. A brief comment here (e.g. // CF=1: carry / unsigned below) would keep the code consistent and make the flag semantics self-documenting.
zhouguangyuan0718
changed the title
zhouguangyuan0718
changed the title
zhouguangyuan0718
changed the title
2 participants
Summary
MOVBLZX,MOVWLZX,MOVWQZX,MOVWQSX,TZCNTQ,BTSQ,BEXTRQ,BZHIQ,SARL,SHLB,JS,JNS, andJNAADDB/SHLB/SET*forms that target low-byte lanes through generic registers likeR10,R11, andR12*amd64*.sfile under/home/zgy/go/pkg/mod/github.com/klauspost/compress@v1.17.9now has no remaining unsupported opcodes by static opcode-to-lowering comparisonTesting
go test ./... -run 'TestAMD64(CmpBtCoverage|MovSyscallAndCRC32Coverage|ArithmeticCoverage|BranchCoverageDeep|CtxAliasAndFPFallbackCoverage|SetCSUsesCarryFlag|SetGEUsesSignedFlag|ADCBUsesCarryFlag)$'*amd64*.sfiles undergithub.com/klauspost/compress@v1.17.9now reportsUNSUPPORTED: (none)Notes
go test ./...still hits existing unrelated failures inTestGoTranslateSignatureCoverageandTestStdlibHashCRC32_AMD64_Compilein this environment