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

Spec 1.3. ORFPX64A32 updates #3

Merged
merged 4 commits into from Jun 6, 2019
Merged

Conversation

bandvig
Copy link
Contributor

@bandvig bandvig commented May 26, 2019

  1. Update pages of all lf.*.d instructions with ORFPX64A32 variant.
  2. Fixed various misprints ans wrong styles.

…2 variant. 2. Fixed various misprints and wrong styles.
@bandvig
Copy link
Contributor Author

bandvig commented May 26, 2019

@wallento @skristiansson @juliusbaxter @jeremybennett @olofk @fjullien @stffrdhrn
Please, find and review my updates for spec-1.3 initially started by @stffrdhrn . Strange, but GitHub didn’t offer me to request review with it’s web-interface, that’s why I had just to mention all of you in the additional comment.

@stffrdhrn
Copy link
Member

@bandvig , this looks good to me. I think having 32-bit implementations of the double-precision instructions inline looks fine. I was thinking it might be overwhelming, but you have done it in a subtle way.

One thing, can you fix up the case of "Double-precision" to "double-precision" on page 180.
Support for Double-precision floating point... Its my typo, I think its better to keep lower.

…lf.stod.d` and `lf.dtos.d` pages. (2) Fix `ls` prefixes by `lf` in chapter ORFPX64A32.
@bandvig
Copy link
Contributor Author

bandvig commented May 27, 2019

One thing, can you fix up the case of "Double-precision" to "double-precision" on page 180.

Done.

@stffrdhrn
Copy link
Member

Is there anything else we want to work on? Some check points in general for me:

  • Should we somewhere mention that ORFPX64A32 is optional?
  • Should we have some kind of UPR spr to indicate support for ORFPX64A32 is available in the processor?

@bandvig
Copy link
Contributor Author

bandvig commented May 30, 2019

Is there anything else we want to work on?

What about to re-arrange instructions classes? On the one hand, I'm not sure that it is actual theme for spec-1.3. On the other hand I remember that initially you planned -mclass options for GCC rather than options like -mcmov e.t.c. Perhaps, we should complete binutils/gcc submits as is and postpone such re-arrangement for next iteration.
I'm going to prepare post into mailing list with my proposal about that. My preliminary vision:

  • Three classes. Class I, II, III (less than in Julius's proposal)
  • Move l.*sync, l.trap and l.sf*i (and perhaps l.cmov and l.ext[bhw]*) into Class-I.
  • FP-instruction almost all in Class-I, including MUL/DIV, but excluding lf.mac*.* and lf.rem.*.
  • Vector-instructions almost all in Class-I, including MUL/DIV, but excluding MACs
  • Class-II (makes sens for ORBIS32/64 only): l.mul* (but not l.muld), l.div*, l.ff1, l.fl1, l.ror, l.rori (and perhaps l.cmov and l.ext[bhw]* if not moved into Class-I), excluding MACs.
  • Class-III: all MACs and lf.rem.* (fairly speaking I'm definitely want to remove lf.rem.* from spec)
  • l.cust* are not classes, they are eXtentions. Their instructions classification is up to designer of the eXtention.

With such classification, GCC options could be:

  1. For 32-bit machine -mor32:
    1a) -morbis=class1/class2/class3
    1b) -morfpx32=class1/class3
    1c) -morfpx64a32=class1/class3

  2. For 64-bit machine -mor64:
    2a) -morbis=class1/class2/class3
    2b) -morfpx=class1/class3
    2c) -morvdx=class1/class3

  • Should we somewhere mention that ORFPX64A32 is optional?

Please, clarify what you mean.
I think it should be described in the same way as ORFPX32/64 eXtensions. As ORFPX32/64 eXtensions is optional by default (and its optionally status is not specially labelled) I think we should do the same for ORFPX64A32. Btw, I've went through spec once more time and found that ORFPX64A32 should be mentioned at least in chapters 2.1 Features and 5.1 Features. I'll add appropriate words soon.

  • Should we have some kind of UPR spr to indicate support for ORFPX64A32 is available in the processor?

I agree. Btw, currently neither FPU nor Vector Unit mentioned in UPR. I would propose the following:

  • UPR[11]: FPU present

  • UPR[12]: Vector Unit present

  • CPUCFGR[15]: OF64A32S ORFPX64A32 Supported / 0 Not supported / 1 Supported

  • CPUCFGR[18],[17],[16]: (or another ?) could indicate which instruction classes are supported.

@bandvig
Copy link
Contributor Author

bandvig commented May 31, 2019

@stffrdhrn
I think my previous proposal about:

  • UPR[11]: FPU present
  • UPR[12]: Vector Unit present

is redundant. It would be enough to indicate ORFPX64A32 presence with just CPUCFGR[15] bit analogously to CPUCFGR[9...7] .

@bandvig
Copy link
Contributor Author

bandvig commented May 31, 2019

@wallento @skristiansson @juliusbaxter @jeremybennett @olofk @fjullien @stffrdhrn

I think we should clarify behavior of l.div* if result isn't integral number. C/C++ (FORTRAN ? :)) assume truncation, that is equivalently that reminder should have same sign as dividend:

7/3 = 2 x 3 + 1
(-7)/3 = (-2) x 3 + (-1)
7/(-3) = (-2) x (-3) + 1
(-7)/(-3) = 2 x (-3) + (-1)

If I understand correctly OR1K's l.div* implement exactly the approach. Than just for clarification we should add something like If the result isn't an integral number it's fractional part should be truncated. on l.div* pages.

PS. Please, don't hesitate to correct my English :).

@stffrdhrn
Copy link
Member

@stffrdhrn
I think my previous proposal about:

  • UPR[11]: FPU present
  • UPR[12]: Vector Unit present

is redundant. It would be enough to indicate ORFPX64A32 presence with just CPUCFGR[15] bit analogously to CPUCFGR[9...7] .

That makes sense, no need to change upr. Just cpucfgr. Also I agree we should remove lf.rem.d, lf.rem.s it's not implemented in any hardware and near impossible to do.

…s 2.1 and 5.1. (2) Add note about truncation fractional part of integer division results for `l.div` and `l.divu`. (3) Define CPUCFGR[15] bit as ORFPX64A32 presence flag. (4) Remove `lf.rem.*` (5) Update all indexes
@stffrdhrn
Copy link
Member

stffrdhrn commented Jun 2, 2019

@bandvig
Hello, the recent updates look good.
I would make one change though. "If the result isn't an integral number then the fractional part should be truncated."

Other than that it looks good. And I think all my concerns are addressed. I will make a web page update.

In terms of the GCC patch upstreaming.

  • GCC - I fixed up all the ChangeLogs and descriptions, so it should be good
  • Binutils - The FPU and Unordered patches are separate, I need to add descriptions and ChangeLogs. Also, I want to add a test case for the l.adrp instruction.
  • CGEN - I added unordered support to it, to support binutils, patches submitted waiting for review.

@bandvig
Copy link
Contributor Author

bandvig commented Jun 4, 2019

@stffrdhrn

In terms of the GCC patch upstreaming.

  • GCC - I fixed up all the ChangeLogs and descriptions, so it should be good
  • Binutils - The FPU and Unordered patches are separate, I need to add descriptions and ChangeLogs. Also, I want to add a test case for the l.adrp instruction.
  • CGEN - I added unordered support to it, to support binutils, patches submitted waiting for review.

Good news. I believe all OR community is looking forward the upstreaming.

@stffrdhrn
Copy link
Member

Shall we merge this? It would be good to get blessing from @wallento @skristiansson @juliusbaxter @olofk . But we can do that after merging this too. Please see the web page PR openrisc/openrisc.github.io#13

@stffrdhrn stffrdhrn merged commit bae182e into openrisc:master Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants