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/compile: apparently extraneous inlmarks #31618

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

Comments

Projects
None yet
4 participants
@josharian
Copy link
Contributor

commented Apr 22, 2019

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 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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

(code from discussion at #31586)

@randall77

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

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

This comment has been minimized.

Copy link

commented Apr 23, 2019

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

@dr2chase

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

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

@gopherbot gopherbot closed this in fd788a8 Apr 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.