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.

Sign up for GitHub

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

build,ir,printer: add InternalFunction #443

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

build,ir,printer: add InternalFunction #443

wants to merge 1 commit into from

Conversation

Garrett-Bodley
Copy link

Adds support for internal assembly functions. The new InternalFunction() call outputs a TEXT directive sans the pesky unicode dot used in Go assembly. This allows Avo to generate an internal assembly function that is not linked to any symbols in the corresponding package.

Closes #442

@Garrett-Bodley
build,ir,printer: add InternalFunction
82f991c 
Adds support for internal assembly functions. The new InternalFunction()
call outputs a TEXT directive sans the pesky unicode dot used in Go
assembly. This allows Avo to generate an internal assembly function that
is not linked to any symbols in the corresponding package.

Closes mmcloughlin#442
@FiloSottile
Copy link

It might be worth making these always static (ending in <> like TEXT p256SqrInternal<>(SB),NOSPLIT,$0) so that they don't conflict with symbols with the same name in other packages or modules.

@Garrett-Bodley
Copy link
Author

There are a number of internal functions in crypto/internal/nistec/p256_asm_amd64.s that lack the static <> characters. I wasn't sure if that was an intentional decision or not.

Making all internal functions static in Avo would result in a diff on those lines when I port that file to Avo, but maybe that is desirable? If those symbols are non-static for a reason then we should leave the PR as is.

I'm fine with either option just wanted to flag that before a decision was made.

If we make all internal functions Static:

Could make sense to make the name field of the Function struct a Symbol instead of the current string

avo/ir/ir.go

Lines 172 to 190 in 13668f4

type Function struct {
Name string
Attributes attr.Attribute
Pragmas []Pragma
Doc []string
Signature *gotypes.Signature
LocalSize int
Nodes []Node
// LabelTarget maps from label name to the following instruction.
LabelTarget map[Label]*Instruction
// Register allocation.
Allocation reg.Allocation
// ISA is the list of required instruction set extensions.
ISA []string
}

The Symbol struct already has a static field and some associated printing logic.

avo/operand/types.go

Lines 14 to 31 in 13668f4

// Symbol represents a symbol name.
type Symbol struct {
Name string
Static bool // only visible in current source file
}
// NewStaticSymbol builds a static Symbol. Static symbols are only visible in the current source file.
func NewStaticSymbol(name string) Symbol {
return Symbol{Name: name, Static: true}
}
func (s Symbol) String() string {
n := s.Name
if s.Static {
n += "<>"
}
return n
}

Happy to add that to this PR

@mmcloughlin do you have thoughts?

Examples of non-static internal functions:

https://github.com/golang/go/blob/27093581b2828a2752a6d2711def09517eb2513b/src/crypto/internal/nistec/p256_asm_amd64.s#L1318
https://github.com/golang/go/blob/27093581b2828a2752a6d2711def09517eb2513b/src/crypto/internal/nistec/p256_asm_amd64.s#L1344
https://github.com/golang/go/blob/27093581b2828a2752a6d2711def09517eb2513b/src/crypto/internal/nistec/p256_asm_amd64.s#L1527
https://github.com/golang/go/blob/27093581b2828a2752a6d2711def09517eb2513b/src/crypto/internal/nistec/p256_asm_amd64.s#L1996

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

build,ir, printer: Support for internal assembly functions
2 participants
@Garrett-Bodley @FiloSottile