Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/internal/obj/x86: remove code marked with TODO #24734

Closed
quasilyte opened this issue Apr 6, 2018 · 1 comment
Closed

cmd/internal/obj/x86: remove code marked with TODO #24734

quasilyte opened this issue Apr 6, 2018 · 1 comment
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@quasilyte
Copy link
Contributor

There are two TODO notes in obj/x86 doasm that I was planning to resolve for quite a long time.

TODO 1/2

// TODO(rsc): This special case is for PINSRQ etc, CMPSD etc.
// Change encoding generated by assemblers and compilers (if any) and remove.
switch p.As {
case AIMUL3Q, APEXTRW, APINSRW, APINSRD, APINSRQ, APSHUFHW, APSHUFL, APSHUFW, ASHUFPD, ASHUFPS, AAESKEYGENASSIST, APSHUFD, APCLMULQDQ:
if p.From3Type() == obj.TYPE_NONE {
p.SetFrom3(p.From)
p.From = obj.Addr{}
p.From.Type = obj.TYPE_CONST
p.From.Offset = p.To.Offset
p.To.Offset = 0
}
case ACMPSD, ACMPSS, ACMPPS, ACMPPD:
if p.From3Type() == obj.TYPE_NONE {
p.SetFrom3(p.To)
p.To = obj.Addr{}
p.To.Type = obj.TYPE_CONST
p.To.Offset = p.GetFrom3().Offset
p.GetFrom3().Offset = 0
}
case AVGATHERDPD,
AVGATHERQPD,
AVGATHERDPS,
AVGATHERQPS,
AVPGATHERDD,
AVPGATHERQD,
AVPGATHERDQ,
AVPGATHERQQ:
if !avx2gatherValid(ctxt, p) {
return
}
}

(AVX2 gather checks share same switch but do not belong to TODO note.)

On tip, you can't make assembler to enter either of those two cases during make.bash,
but with 0bf79b2d checked out, we can see the effects and consequences.

For example if case with PINSRQ is commented-out, this error will occur:

asm: doasm: notfound ft=14 tt=63 00006 (~/go/src/runtime/asm_amd64.s:920)	PINSRQ	CX, $1,X6 14 63

doasm does "fix" operands for some instructions because assembler frontend
fills obj.Prog in unusual way:

case '6', '8':
	// CMPSD etc.; third operand is imm8, stored in offset, or a register.
	prog.From = a[0]
	prog.To = a[1]
	switch a[2].Type {
	case obj.TYPE_MEM:
		prog.To.Offset = p.getConstant(prog, op, &a[2])
	case obj.TYPE_REG:
		// Strange reordering.
		prog.To = a[2]
		prog.From = a[1]
		prog.To.Offset = p.getImmediate(prog, op, &a[0])
	default:
		p.errorf("expected offset or register for 3rd operand")
	}

Crawling towards master will lead to 845c4ff CL6901.

Among other things, it does change lines showed above:

                case '6', '8':
-                       // CMPSD etc.; third operand is imm8, stored in offset, or a register.
                        prog.From = a[0]
-                       prog.To = a[1]
-                       switch a[2].Type {
-                       case obj.TYPE_MEM:
-                               prog.To.Offset = p.getConstant(prog, op, &a[2])
-                       case obj.TYPE_REG:
-                               // Strange reordering.
-                               prog.To = a[2]
-                               prog.From = a[1]
-                               prog.To.Offset = p.getImmediate(prog, op, &a[0])
-                       default:
-                               p.errorf("expected offset or register for 3rd operand")
-                       }
+                       prog.From3 = a[1]
+                       prog.To = a[2]

This alone is enough to remove code referenced in TODO-1.

TODO 2/2

Second TODO is trickier.
It's also obsolete due to CL6901, but in order to verify that, more actions are required.

// TODO(rsc): This special case is for SHRQ $3, AX:DX,
// which encodes as SHRQ $32(DX*0), AX.
// Similarly SHRQ CX, AX:DX is really SHRQ CX(DX*0), AX.
// Change encoding generated by assemblers and compilers and remove.
if (p.From.Type == obj.TYPE_CONST || p.From.Type == obj.TYPE_REG) && p.From.Index != REG_NONE && p.From.Scale == 0 {
p.SetFrom3(obj.Addr{
Type: obj.TYPE_REG,
Reg: p.From.Index,
})
p.From.Index = 0
}

Given this example code:

TEXT asmtest(SB), 0, $0
        SHLQ $3, AX:DX
        RET

We can instrument assembler to print something when it enters branch for special case.
Suppose we are printing p stringified representation before and after if body:

inst before: 00000 (foo.s:2)	SHLQ	$3, AX
inst after: 00000 (foo.s:2)	SHLQ	$3, DX, AX

(New "AX:DX" => "DX, AX" syntax is a feature.)

The main thing to observe is that asm code above does trigger that
special condition at previous commit (845c4ff52a624a61be5a0669ec315b6f3a651b51~1 / 59584ed)
and it does not trigger with that revision applied.

Still, running make.bash will fail without that code.
Why?
Because older 6a asm uses older format for those.
Commit ba0c142 CL12784 removes 6a and
we can do make.bash without any problems even with that code removed.

CL is on it's way.
This issue will be referenced to provide information that can't be conveniently
represented inside commit message.

@gopherbot
Copy link

Change https://golang.org/cl/105156 mentions this issue: cmd/internal/obj/x86: remove redundant code (fix TODO)

@bcmills bcmills added this to the Go1.11 milestone Apr 6, 2018
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 6, 2018
@golang golang locked and limited conversation to collaborators Apr 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants