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/asm: add missing s390x branch/move on condition suffixes and improve consistency #19483

Closed
mundaym opened this issue Mar 9, 2017 · 5 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@mundaym
Copy link
Member

mundaym commented Mar 9, 2017

I'd like the compiler to be able to generate test-under-mask instructions (TMLL, TMLH, TMHL and TMHH) for s390x. To do this properly the SSA backend needs to be able to deal with (and be able to reverse) all 14 useful condition codes, rather than just the 9 it currently handles. We don't currently have suffixes for all these condition codes and in the process of adding them I'm wondering whether to also try and make the existing ones more consistent with their GNU assembler (GAS) equivalents.

The exact meaning of the bits set in the condition code vary from instruction to instruction. For example, bit 3 set by test-under-mask means 'selected bits are all ones'. However in general they correspond to the following (big endian, so 0 is MSB, 3 is LSB):

  1. equal
  2. less than
  3. greater than
  4. overflow (also commonly used to represent 'unordered' for floating point comparisons)

Here are my proposed changes:

Condition CodeGAS SuffixCurrent Go SuffixProposed Go Suffix
0x0VC
0x1oVSO (overflow)
0x2hGTGT (greater than)
0x3nleNLE (not less than and not equal)
0x4lLTLT (less than)
0x5nheLTUNGE (not greater than and not equal)
0x6lhLG (less or greater than)
0x7neNENE (not equal)
0x8eEQEQ (equal)
0x9nlhNLG (not less than and not greater than)
0xaheGEGE (greater than or equal)
0xbnlNL (not less than)
0xcleLELE (less than or equal)
0xdnhLEUNG (not greater than)
0xenoNO (no overflow)
0xf

I also like the idea of making each suffix exactly two characters long, but I suspect it is better to stick more closely to GAS. If every suffix were to be two characters, I would substitute as follows:

  • O -> OV (overflow)
  • NGE -> LO (less than or overflow)
  • NLE -> GO (greater than or overflow)
  • NLG -> EO (equal or overflow)

We'd need to decide whether to keep the current mnemonics around as aliases or not. I think we should keep BVS but the rest can probably go.

We could replace GT with H, EQ with E and so on but that feels like a step too far. These suffixes are more widely used than the overflow ones and H would conflict with our uses of H for half-word.

@bradfitz bradfitz added this to the Go1.9Maybe milestone Mar 21, 2017
@bradfitz
Copy link
Contributor

@mundaym, this breaks s390x assembly code in the wild, right? The go1compat promise doesn't cover that, so if you're all cool with this at IBM, it's up to you.

@mundaym
Copy link
Member Author

mundaym commented May 3, 2017

The only s390x assembly that I know of lives in the Go standard library, so it can be updated at the same time. We'll probably add some to x/crypto at some point, but there isn't any there yet. So I don't think s390x asm compatibility is currently that important in practice, particularly for the less heavily used mnemonics. Having said that I might just keep the old mnemonics around anyway when/if I do eventually get round to submitting this change, just in case.

I've held off on this because allowing the compiler to use all these modes adds a lot of complexity to the SSA rules and it isn't completely clear to me that it is worth it yet. I'll move this to the unplanned milestone for now.

@mundaym mundaym modified the milestones: Unplanned, Go1.9Maybe May 3, 2017
@bradfitz
Copy link
Contributor

bradfitz commented May 3, 2017

SGTM. Feel free to make the "breaking" change & remilestone to Go 1.10 if you'd like.

@gopherbot
Copy link

Change https://golang.org/cl/38008 mentions this issue: cmd, math: add missing s390x branch/conditional move mnemonics

@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 26, 2019
@mundaym
Copy link
Member Author

mundaym commented Oct 10, 2019

CL 196558 adds mnemonics for these instructions that allow the condition code mask to be manually specified (e.g. rather than using the EQ suffix you can supply the equivalent condition code mask ,$8). This is just as flexible as the extended mnemonics so I don't think it is worth adding the extended mnemonics at this time.

@mundaym mundaym closed this as completed Oct 10, 2019
@golang golang locked and limited conversation to collaborators Oct 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

4 participants