Interaction between bitfields and casts

Interaction between bitfields and casts

[Dibyendu Majumdar]

Hi,

When assigning values to bitfields I have noticed that some times
Sparse generates a CAST instruction with an odd bit size - i.e. not a
standard integer size. I was wondering if there are specific scenarios
that lead to this. While LLVM handles the odd cast - I am working with
another backend (Nanojit) that doesn't.

Regards
Dibyendu

Re: Interaction between bitfields and casts

[Luc Van Oostenryck]

On Wed, Aug 9, 2017 at 10:41 PM, Dibyendu Majumdar
<mobile@majumdar.org.uk> wrote:
> Hi,
>
> When assigning values to bitfields I have noticed that some times
> Sparse generates a CAST instruction with an odd bit size - i.e. not a
> standard integer size. I was wondering if there are specific scenarios
> that lead to this. While LLVM handles the odd cast - I am working with
> another backend (Nanojit) that doesn't.

Yes, I know. There is already one less with:
    [PATCH v4 5/9] change the masking when loading bitfields"

Otherwise, it's really a question of translating then into the right
instruction(s) to truncate/zero extend/sign extend part of a word
(but the casts instructions in sparse's IR are a bit strange and
need special care when the src or the dest is not an integer but
a float or a pointer).

I have an unfinished series that clean this a bit but
it will be after the currently pending series.

-- Luc

Re: Interaction between bitfields and casts

[Linus Torvalds]

On Wed, Aug 9, 2017 at 1:41 PM, Dibyendu Majumdar
<mobile@majumdar.org.uk> wrote:
>
> When assigning values to bitfields I have noticed that some times
> Sparse generates a CAST instruction with an odd bit size - i.e. not a
> standard integer size. I was wondering if there are specific scenarios
> that lead to this. While LLVM handles the odd cast - I am working with
> another backend (Nanojit) that doesn't.

The cast instructions are a bit odd, I agree.

The basic rule is that each pseudo has a type (signed int, unsigned
int, fp, ptr) and a size (which is whatever, although obviously
pointers and FP values are fairly constrained).

NOTE! By "type" above, I mean "value type". There's another kind of
"type" when it comes to pseudos, ie the "pseudo->type" thing, which is
reg/sym/val/arg/phi. That's a different thing entirely.

Anyway, the size and value type of a pseudo is defined by the size of
the instruction that defines it.

And whenever the type or size changes, there should be a cast instruction.

But honestly, the casts are not very well done.

The size change should always be fairly obvious: you can see the old
size by looking at the size of the defining instruction of the source
pseudo, and you can see the new size by looking at the size of the
cast instruction that defines the new pseudo.

But the "value type" change is pretty garbage. We have four "ops"
(CAST/SCAST/PTRCAST/FPCAST), but they make absolutely no sense. The
problem is that there really are many many different combinations
(unsigned integer to floating point, Fp to ptr, etc etc), and the
opcode is generated from some random combination of source/dst that
really doesn't make sense.

I'm not proud of it. It probably really should be just one cast op,
and then the type should be something that - like the size - is
something you go look up.

And *when* the source and destination are both integers (and arguably
a pointer too), then obviously the cast can just always be turned into
a zero-extension or sign extension.

So the OP_*CAST functions really are a bit confused. Maybe Luc has
cleaned some of it up already, I'm going partly from memory.

To make a long story short: the OP_*CAST stuff is not great, and
doesn't make a ton of sense. It should probably be rewritten.

                     Linus

Re: Interaction between bitfields and casts

[Luc Van Oostenryck]

On Wed, Aug 9, 2017 at 11:27 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:

> So the OP_*CAST functions really are a bit confused. Maybe Luc has
> cleaned some of it up already, I'm going partly from memory.

I have but I never posted it yet.
Basically, I have specialized them:
- TRUNC, ZEXT & SEXT: truncate (may be not neede), zero & sign extend
for integers
- PTRU & UPTR: conversion pointer from/to unsigned integer (not sure
it's needed)
- FCVTU & FCVTS: convert float to unsigned/signed integers
- UCVTF & SCVTF: convert int to float
- FCVTF: float to float convert (maybe even better/simpler with FTRUNC
& FEXT ?à)

The main objective being to make things more explicit a avoid to have
to test the orig-type at each time.

-- Luc

Re: Interaction between bitfields and casts

[Linus Torvalds]

On Wed, Aug 9, 2017 at 3:06 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> I have but I never posted it yet.
> Basically, I have specialized them:
> - TRUNC, ZEXT & SEXT: truncate (may be not neede), zero & sign extend
> for integers

Makes sense.

And yes, I think we could skip TRUNC (casting to a smaller type is one
of those questionable things we do now).

Our IR doesn't really have any well-defined form for upper bits anyway
(ie there's no guarantee that values are zero-extended or
sign-extended to the full register width in the IR), so "TRUNC"
doesn't really have any sane semantics.

> - PTRU & UPTR: conversion pointer from/to unsigned integer (not sure
> it's needed)

So this is a separate thing that doesn't take a size?

So a "int to ptr" conversion on a 64-bit architecture would be a
combination of SEXT.64 and a UPTR?

If so, then yes, I think that's the right thing to do (and then in
basically all cases the PTRU/UPTR just goes away).

Or, like TRUNC, just say that pointers simply *are* the same thing as
an integer of the same size, and PTRU/UPTR just doesn't exist at all.

> - FCVTU & FCVTS: convert float to unsigned/signed integers
> - UCVTF & SCVTF: convert int to float
> - FCVTF: float to float convert (maybe even better/simpler with FTRUNC
> & FEXT ?à)
>
> The main objective being to make things more explicit a avoid to have
> to test the orig-type at each time.

Yes. I think what you describe is the right thing to do. The current
casting is just wrong and nasty, and has too much implicit stuff.

                    Linus

Re: Interaction between bitfields and casts

[Luc Van Oostenryck]

On Thu, Aug 10, 2017 at 12:44 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Aug 9, 2017 at 3:06 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
>>
>> I have but I never posted it yet.
>> Basically, I have specialized them:
>> - TRUNC, ZEXT & SEXT: truncate (may be not neede), zero & sign extend
>> for integers
>
> Makes sense.
>
> And yes, I think we could skip TRUNC (casting to a smaller type is one
> of those questionable things we do now).

Maybe it can be useful for a short time after linearization, if we do some
type analysis or so, I'm not sure.
But after a while, they must be simplified away or converted to an AND mask.

> Our IR doesn't really have any well-defined form for upper bits anyway
> (ie there's no guarantee that values are zero-extended or
> sign-extended to the full register width in the IR), so "TRUNC"
> doesn't really have any sane semantics.
>
>> - PTRU & UPTR: conversion pointer from/to unsigned integer (not sure
>> it's needed)
>
> So this is a separate thing that doesn't take a size?
>
> So a "int to ptr" conversion on a 64-bit architecture would be a
> combination of SEXT.64 and a UPTR?

Yes, it's the idea. Basically a no-op.

> If so, then yes, I think that's the right thing to do (and then in
> basically all cases the PTRU/UPTR just goes away).
>
> Or, like TRUNC, just say that pointers simply *are* the same thing as
> an integer of the same size, and PTRU/UPTR just doesn't exist at all.

Same as for TRUNC, I think. At the end it's sure we don't want them,
they are just the same as unsigned integers of the same size.
But maybe during early phases, we want to keep them as such
because we need to know they correspond to a pointer or an address.

>> - FCVTU & FCVTS: convert float to unsigned/signed integers
>> - UCVTF & SCVTF: convert int to float
>> - FCVTF: float to float convert (maybe even better/simpler with FTRUNC
>> & FEXT ?à)
>>
>> The main objective being to make things more explicit a avoid to have
>> to test the orig-type at each time.
>
> Yes. I think what you describe is the right thing to do. The current
> casting is just wrong and nasty, and has too much implicit stuff.

Yes, the missing of an explicit information about the signed/unsigned
is quite error-prone.

-- Luc

[SPARSE] cast rework (was: Re: Interaction between bitfields and casts)

[Luc Van Oostenryck]

On Wed, Aug 09, 2017 at 03:44:02PM -0700, Linus Torvalds wrote:
> On Wed, Aug 9, 2017 at 3:06 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > I have but I never posted it yet.
> > Basically, I have specialized them:
> > - TRUNC, ZEXT & SEXT: truncate (may be not neede), zero & sign extend
> > for integers
> 
> Makes sense.
> 
> And yes, I think we could skip TRUNC (casting to a smaller type is one
> of those questionable things we do now).
> 
> Our IR doesn't really have any well-defined form for upper bits anyway
> (ie there's no guarantee that values are zero-extended or
> sign-extended to the full register width in the IR), so "TRUNC"
> doesn't really have any sane semantics.
> 
> > - PTRU & UPTR: conversion pointer from/to unsigned integer (not sure
> > it's needed)
> 
> So this is a separate thing that doesn't take a size?
> 
> So a "int to ptr" conversion on a 64-bit architecture would be a
> combination of SEXT.64 and a UPTR?
> 
> If so, then yes, I think that's the right thing to do (and then in
> basically all cases the PTRU/UPTR just goes away).
> 
> Or, like TRUNC, just say that pointers simply *are* the same thing as
> an integer of the same size, and PTRU/UPTR just doesn't exist at all.
> 
> > - FCVTU & FCVTS: convert float to unsigned/signed integers
> > - UCVTF & SCVTF: convert int to float
> > - FCVTF: float to float convert (maybe even better/simpler with FTRUNC
> > & FEXT ?à)
> >
> > The main objective being to make things more explicit a avoid to have
> > to test the orig-type at each time.
> 
> Yes. I think what you describe is the right thing to do. The current
> casting is just wrong and nasty, and has too much implicit stuff.
> 
>                     Linus

I'm busy to test & polish the patch series corresponding to this and
I discovered an annoying oddball:
   Cast to an union (where the union contains a field of the same
   type as the source).
This is a GCC extension and unlike a normal cast it produces an l-value.
The kernel uses this a few times (at least once as an l-value).
It will need a bit work on sparse side to handle this.

-- Luc