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

Jump to bottom

cmd/compile: apparently extraneous inlmarks #31618

Closed
josharian opened this issue Apr 22, 2019 · 5 comments
Closed

cmd/compile: apparently extraneous inlmarks #31618

josharian opened this issue Apr 22, 2019 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Go1.13

Comments

@josharian
Copy link
Contributor 

package p

import "encoding/binary"

func f(key [4]byte, b []byte) {
	k := binary.LittleEndian.Uint32(key[:])
	v := binary.LittleEndian.Uint32(b)
	binary.LittleEndian.PutUint32(b, v^k)
}

generates:

"".f STEXT nosplit size=62 args=0x20 locals=0x18
	0x0000 00000 (x.go:5)	TEXT	"".f(SB), NOSPLIT|ABIInternal, $24-32
	0x0000 00000 (x.go:5)	SUBQ	$24, SP
	0x0004 00004 (x.go:5)	MOVQ	BP, 16(SP)
	0x0009 00009 (x.go:5)	LEAQ	16(SP), BP
	0x000e 00014 (x.go:5)	FUNCDATA	$0, gclocals·09cf9819fc716118c209c2d2155a3632(SB)
	0x000e 00014 (x.go:5)	FUNCDATA	$1, gclocals·69c1753bd5f81501d95132d08af04464(SB)
	0x000e 00014 (x.go:5)	FUNCDATA	$2, gclocals·9fb7f0986f647f17cb53dda1484e0f7a(SB)
	0x000e 00014 (x.go:6)	PCDATA	$0, $0
	0x000e 00014 (x.go:6)	PCDATA	$1, $0
	0x000e 00014 (x.go:6)	XCHGL	AX, AX
	0x000f 00015 (x.go:7)	XCHGL	AX, AX
	0x0010 00016 (x.go:6)	MOVL	"".key+32(SP), DX
	0x0014 00020 ($GOROOT/src/encoding/binary/binary.go:63)	MOVQ	"".b+48(SP), CX
	0x0019 00025 ($GOROOT/src/encoding/binary/binary.go:63)	CMPQ	CX, $3
	0x001d 00029 ($GOROOT/src/encoding/binary/binary.go:63)	JLS	51
	0x001f 00031 (x.go:8)	XCHGL	AX, AX
	0x0020 00032 (x.go:8)	PCDATA	$0, $1
	0x0020 00032 (x.go:8)	PCDATA	$1, $1
	0x0020 00032 (x.go:8)	MOVQ	"".b+40(SP), AX
	0x0025 00037 (x.go:8)	XORL	(AX), DX
	0x0027 00039 ($GOROOT/src/encoding/binary/binary.go:72)	PCDATA	$0, $0
	0x0027 00039 ($GOROOT/src/encoding/binary/binary.go:72)	MOVL	DX, (AX)
	0x0029 00041 (<unknown line number>)	MOVQ	16(SP), BP
	0x002e 00046 (<unknown line number>)	ADDQ	$24, SP
	0x0032 00050 (<unknown line number>)	RET
	0x0033 00051 ($GOROOT/src/encoding/binary/binary.go:63)	MOVL	$3, AX
	0x0038 00056 ($GOROOT/src/encoding/binary/binary.go:63)	CALL	runtime.panicIndex(SB)
	0x003d 00061 ($GOROOT/src/encoding/binary/binary.go:63)	XCHGL	AX, AX

Note the side-by-side NOPs at 0x000e and 0x000f. (They do have different line numbers.) They are also at the beginning of the routine, before any other work has been done.

This makes me wonder whether we could eliminate either or both. @randall77 anything we can do here?
@josharian josharian added Performance NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 22, 2019
@josharian josharian added this to the Go1.13 milestone Apr 22, 2019
@josharian josharian changed the title cmd/compile: apparent extraneous inlmarks cmd/compile: apparently extraneous inlmarks Apr 22, 2019
@josharian
Copy link
Contributor Author

(code from discussion at #31586)

@randall77
Copy link
Contributor

This is basically a dup of #29571. The two marks are to remember the positions of the two callsites.

I'm surprised we don't remove the first nop. There's another instruction with the same line number - we should have used that instruction instead for the mark, as of CL 158021.

We could put these nops at the end of the function, so they never get executed. I put them inline because it might help debuggers set a breakpoint on that source line. But that's just from my intuition - I'm not sure if that's in fact the case, or if there is another way (DWARF, I guess) to enable that.

@randall77
Copy link
Contributor

Looks like CL 169739 is the one that broke CL 158021. The line numbers don't match because one is a statement and one isn't.

@dr2chase : What's the right way to do atColumn1? I think we want the result to always be a statement, as (if we keep it) it will always be a standin for a real line of code, and always be the only instruction at that line. But generally, for this application we just need it to always return equal Poses given two instructions that are at the same line.

Would makeLico(x.Line(), 1).withIsStmt() work?

@gopherbot
Copy link

Change https://golang.org/cl/173324 mentions this issue: cmd/compile: always mark atColumn1 results as statements

@dr2chase
Copy link
Contributor

I think that's a good CL, and the reasoning for make it be a statement is sound.

@golang golang locked and limited conversation to collaborators Apr 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
4 participants
@josharian @dr2chase @randall77 @gopherbot