Bug 684 - XLEN-16 fails when XLEN=8
Summary: XLEN-16 fails when XLEN=8
Status: CONFIRMED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Source Code (show other bugs)
Version: unspecified
Hardware: PC Linux
: --- enhancement
Assignee: Dmitry Selyutin
URL: https://libre-soc.org/openpower/sv/sv...
Depends on:
Blocks: 671
  Show dependency treegraph
 
Reported: 2021-09-04 18:58 BST by Luke Kenneth Casson Leighton
Modified: 2022-05-12 17:03 BST (History)
3 users (show)

See Also:
NLnet milestone: ---
total budget (EUR) for completion of task and all subtasks: 0
budget (EUR) for this task, excluding subtasks' budget: 0
parent task for budget allocation:
child tasks for budget allocation:
The table of payments (in EUR) for this task; TOML format:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Kenneth Casson Leighton 2021-09-04 18:58:30 BST
https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=42e799cd6ed350b3bd3e6b84c97b9e09c76da0a1

-    if      a <u ([0]*48 || UI) then c <- 0b100
-    else if a >u ([0]*48 || UI) then c <- 0b010
+    if      a <u ([0]*(XLEN-16) || UI) then c <- 0b100
+    else if a >u ([0]*(XLEN-16) || UI) then c <- 0b010

this will go negative.  andi, ori and xori also have the same
issue

https://libre-soc.org/openpower/isa/fixedlogical/
Comment 1 Dmitry Selyutin 2021-09-04 19:19:32 BST
I suggest, as mentioned in IRC logs, to make these operate on 1/4 of the register (so XLEN-XLEN/4).
Comment 2 Luke Kenneth Casson Leighton 2021-09-04 19:28:01 BST
it's not the register itself that's the issue, it's the immediate.
or, when XLEN=8, "[0]*(XLEN-16)" returns a *negative* length for
a list!  -8 zeros?

i've just spotted that xoris, andis and oris will also barf:

    RA <- (RS) ^ ([0]*(XLEN-32) || UI || [0]*16)
    RA <- (RS) & ([0]*(XLEN-32) || UI || [0]*16)

so these need some thought as well.
Comment 3 Dmitry Selyutin 2021-09-04 19:48:59 BST
UI in D-form currently specifies a 16-bit integer. Either it must be re-considered, or we're out of luck. It produces at least 16 bits.
Comment 4 Dmitry Selyutin 2021-09-04 20:17:11 BST
I think the most natural and logical change to fixedlogical (no pun intended) is scaling all things appropriately, i.e.

RA <- (RS) ^ ([0]*32 || UI || [0]*16)

...is converted into this:

RA <- (RS) ^ ([0]*(XLEN/2) || (UI & ([0b1]*(XLEN/4))) || [0]*(XLEN/4))

and this...

if      a <u ([0]*(XLEN-16) || UI) then c <- 0b100
else if a >u ([0]*(XLEN-16) || UI) then c <- 0b010

...is converted in a similar fashion:

if      a <u ([0]*(XLEN-(XLEN/4)) || (UI & ([0b1]*(XLEN/4))) then c <- 0b100
else if a >u ([0]*(XLEN-(XLEN/4)) || (UI & ([0b1]*(XLEN/4))) then c <- 0b010
Comment 5 Luke Kenneth Casson Leighton 2021-09-04 21:47:03 BST
(In reply to dmitry.selyutin from comment #3)
> UI in D-form currently specifies a 16-bit integer. Either it must be
> re-considered, or we're out of luck. It produces at least 16 bits.

i believe it's reasonable to truncate those to 8 bit for XLEN=8
it does however mean an "if" statement:

if XLEN = 8 then
   # and now for something completely different
   # https://www.youtube.com/watch?v=iTUAhkzVa0M&list=PLnCJp8j83vkH0tD6jOnGc5kPCeiml2tG5
Comment 6 Luke Kenneth Casson Leighton 2021-09-04 23:00:28 BST
for andi xori etc. these should all be something like:

if XLEN=8 then
    RA <- (RS) ^ UI[8:15]
else
    RA <- (RS) ^ ([0]*(XLEN-16) || UI)
Comment 7 Luke Kenneth Casson Leighton 2021-09-14 16:42:16 BST
(In reply to Luke Kenneth Casson Leighton from comment #6)
> for andi xori etc. these should all be something like:
> 
> if XLEN=8 then
>     RA <- (RS) ^ UI[8:15]
> else
>     RA <- (RS) ^ ([0]*(XLEN-16) || UI)

i had a bit of a think about this, and feel that this is one
of those exceptional cases where we could justify a function,
to be added to section 1.3.2

the function proposed, in the usual obtuse shortness paradigm,
so loved by spec writers, would be XLT or XTRUNC, and would
be like this:

def XTRUNC(x):
    return SelectableInt(x.value, min(x.bits, XLEN)

where SelectableInt takes care of the required truncation
of the input value.

all locations that risk an overflow when XLEN is less than
the immediate width after zero-extending would be simply:

     RA <- (RS) ^ XTRUNC([0]*(XLEN-16) || UI)

nothing more sophisticated.

this minimises the intrusiveness into the pseudocode, which would
otherwise see significant disruption and lack of clarity.
Comment 8 Jacob Lifshay 2021-09-14 19:01:56 BST
(In reply to Luke Kenneth Casson Leighton from comment #7)
> (In reply to Luke Kenneth Casson Leighton from comment #6)
> > for andi xori etc. these should all be something like:
> > 
> > if XLEN=8 then
> >     RA <- (RS) ^ UI[8:15]
> > else
> >     RA <- (RS) ^ ([0]*(XLEN-16) || UI)
> 
> i had a bit of a think about this, and feel that this is one
> of those exceptional cases where we could justify a function,
> to be added to section 1.3.2

I disagree... the code could be easily written as:
RA <- (RS) ^ ([0]*48 || UI)[63-XLEN:63]

and instructions like xoris would be:
RA <- (RS) ^ ([0]*32 || UI || [0]*16)[63-XLEN:63]

It would not be useful for XLEN <= 16, but still valid. xori would be used instead for XLEN <= 16.
Comment 9 Luke Kenneth Casson Leighton 2021-09-14 19:53:31 BST
(In reply to Jacob Lifshay from comment #8)

> I disagree... the code could be easily written as:
> RA <- (RS) ^ ([0]*48 || UI)[63-XLEN:63]

if it can be made to work for XLEN=128, great.
and is readable, and is not too intrusive a
change.

remember last week's meeting with Paul, he said we
have to find justifications for these fundamental
ISA RFCs.

otherwise IBM Engineers will see our work as hostile
and a total waste of effort, and reject it as a waste
of time.

i posted about this only a few days ago, did you read the
message?


 
> and instructions like xoris would be:
> RA <- (RS) ^ ([0]*32 || UI || [0]*16)[63-XLEN:63]
> 
> It would not be useful for XLEN <= 16, but still valid. xori would be used
> instead for XLEN <= 16.

yeh totally get why.


when converted to cope with a future XLEN=128 i am slightly
concerned about readability, and questions we have to answer.

"why are you adding 1 zeros in front?"
"why are you making unnecessary changes?"
"128 bit scalar has never been discussed in the ISA WG"
"who do you think you are, trying to force us to do extra work?"
"are you proposing XLEN=128 RIGHT NOW? if not, why are you bothering
to do a half-hearted job?"

etc etc. these and many other tough questions will come up.

extending the zeros from 48 to 48+64 i am concerned is harder
to justify and harder to read, it is not quite clear why so
many zeros are added then cut off again.

even changing the umber of zeros at all makes it harder to read.
Comment 10 Luke Kenneth Casson Leighton 2021-09-14 20:13:16 BST
this one needs careful thought.  i cannot say i am happy with any
options (including the XLTRUNC one), whatever is done
though, we have to have good justification for it.
Comment 11 Jacob Lifshay 2021-09-14 20:22:50 BST
(In reply to Luke Kenneth Casson Leighton from comment #10)
> this one needs careful thought.  i cannot say i am happy with any
> options (including the XLTRUNC one), whatever is done
> though, we have to have good justification for it.

if we do add a XLTRUNC function, I think it should truncate/zero-extend to XLEN, so XLTRUNC(UI) is valid for XLEN <, =, or > 16. perhaps we should instead call it XLCASTU, and have a corresponding sign-extending/truncating XLCASTS function.
Comment 12 Jacob Lifshay 2021-09-14 20:24:19 BST
we will also probably want functions for the much-more-complex floating-point XLEN handling
Comment 13 Luke Kenneth Casson Leighton 2021-09-14 21:22:47 BST
(In reply to Jacob Lifshay from comment #11)
> (In reply to Luke Kenneth Casson Leighton from comment #10)

> if we do add a XLTRUNC function, I think it should truncate/zero-extend to
> XLEN, so XLTRUNC(UI) is valid for XLEN <, =, or > 16. perhaps we should
> instead call it XLCASTU, and have a corresponding sign-extending/truncating
> XLCASTS function.

that makes sense

(In reply to Jacob Lifshay from comment #12)
> we will also probably want functions for the much-more-complex
> floating-point XLEN handling

ah these i was planning to just totally cheat, and override the
function names.  e.g. DOUBLE2SINGLE gets redirected to SINGLE2HALF

have to see when we get to it.
Comment 14 Luke Kenneth Casson Leighton 2021-09-30 14:34:13 BST
helpers.py:

+    def XLCASTU(self, value):
+        bits = min(value.bits, self.XLEN)
+        return SelectableInt(value.value & ((1 << bits) - 1), self.XLEN)
+

that can just be:

        return SelectableInt(value.value, self.XLEN)

with a comment explaining that SelectableInt performs the required
value truncation.
Comment 15 Dmitry Selyutin 2022-05-12 17:03:44 BST
This one should also be closed, but I think this one deserves a budget allocation. Which task could be used?