llvm dev - Apr 2018 - llvm::PointerIntPair -- is this by design or a bug?

I'd argue that bitfield sign extensions are surprising and are usually a
source of bugs. It would be much more explicit and less error prone for the
user to write the sign extension if they want it.

By extension, it seems good that PointerIntPair doesn't do sign extension
when the type happens to be signed.

On Wed, Apr 4, 2018 at 9:47 AM David Blaikie via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> I think it'd be reasonable to model this on the same behavior as int to
> short to int round-tripping & not to speculate that there might be code
> relying on the existing behavior until there's evidence of it.
>
> I'd suggest changing the behavior & testing to see if anything
breaks -
> and if nothing does, moving to the behavior rather than supporting both.
>
> On Wed, Apr 4, 2018 at 9:27 AM Riyaz Puthiyapurayil <
> Riyaz.Puthiyapurayil at synopsys.com> wrote:
>
>> Rather than “fixing” it, it might be better to support a separate
method
>> for signed extension. My reasoning is as follows:
>>
>>
>>
>> int x = 7;
>>
>> llvm::PointerIntPair<double*, 3, int> pip;
>>
>> pip.setInt(x);
>>
>>
>>
>> There could be code out there that expects pip.getInt() to return 7 and
>> not -1.
>>
>>
>>
>> So if you really want to set a negative and return a negative value, a
>> separate method setSignedInt and getSignedInt may be OK. Further,
>> sign-extension would need two shift instructions in X86 as opposed to
>> no-sign extension where only one ‘and’ with mask is needed for
retrieving
>> the int.
>>
>>
>>
>> *From:* David Blaikie [mailto:dblaikie at gmail.com]
>> *Sent:* Wednesday, April 4, 2018 7:43 AM
>> *To:* Riyaz Puthiyapurayil <Riyaz.Puthiyapurayil at synopsys.com>
>> *Cc:* Florian Hahn <florian.hahn at arm.com>; llvm-dev <
>> llvm-dev at lists.llvm.org>; nd <nd at arm.com>
>> *Subject:* Re: [llvm-dev] llvm::PointerIntPair -- is this by design or
a
>> bug?
>>
>>
>>
>> I'd suggest someone try fixing this & see if it breaks anything
that
>> can't reasonably be fixed (before we go assuming this is by
>> design/shouldn't be fixed just because it's the way it is
today).
>>
>>
>>
>> On Wed, Apr 4, 2018 at 7:16 AM Riyaz Puthiyapurayil via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>> It won't move the sign bit, so negative values won't fit,
unless you have
>> a 3 bit signed type ;)
>>
>>
>>
>> Note that if you assign negative values to and then read from a signed
>> bit-field, you would do sign extension. So 3-bit signed types do exist
in
>> C++.
>>
>>
>>
>> It begs the question why PointerIntPair supports signed int types if it
>> always loses the sign. Is it just to avoid signed/unsigned comparison
when
>> comparing the return value of getInt with signed types? Or to use enums
>> that default to a signed type? In any case, this should be clearly
>> documented if there is no intention to fix it.
>>
>>
>>
>> / Riyaz
>>
>>
>> On Apr 4, 2018, at 4:04 AM, Florian Hahn <florian.hahn at
arm.com> wrote:
>>
>>
>>
>> On 04/04/2018 11:15, David Chisnall wrote:
>>
>> On 4 Apr 2018, at 11:01, Florian Hahn via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>
>>
>> Hi,
>>
>>
>>
>> On 04/04/2018 05:34, Riyaz Puthiyapurayil via llvm-dev wrote:
>>
>> llvm::PointerIntPair<double*, 3, signed> P;
>>
>> P.setInt(-4);
>>
>> Ideally, the value range for a 3-bit signed integer should be [-4,3].
But
>> the above call to setInt will fail. Essentially, the signed int field
in
>> PointerIntPair is behaving the same as an 3-bit unsigned field which
has
>> the legal value range of [0,7]. Is this by design? Are negative values
not
>> allowed in PointerIntPair?
>>
>>
>>
>> I think that's by design. setInt only allows you to set integer
values
>> that fit into the available bits. It won't move the sign bit, so
negative
>> values won't fit, unless you have a 3 bit signed type ;)
>>
>> That doesn’t sound right (for any computer made in the last few
decades),
>> the representation of -3 will be 1111…1111101.  Storing the low bits
will
>> yield 101, which is a 3-bit negative three.  When you then sign extend
this
>> to any other signed type, you will get -3 in that representation.  It
>> sounds as if the signed specialisation of PointerIntPair is simply not
>> doing the sign extension.
>>
>>
>> Yep, I meant it looks like it currently does not do a sign extension,
it
>> expects only the available bits to be set, no others. In any case, it
is
>> probably worth documenting the behaviour.
>>
>> Cheers,
>> Florian
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
<https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Ddev&d=DwMFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=mMZWMrEZcvPMLSsEQSah9FOTwza1UudSDkAneN47U9lD3qu6gt3kpnIb4MWV77cM&m=g0Y-dsef9q-C3A1mCQrgZOoqoE7bFtitv8BNSqw9JNY&s=Ka0s2M91ZtQxhGGXhq0Wuf99yfStGshbKPwCryYAqEg&e=>
>>
>> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180404/35274443/attachment-0001.html>

The sign extension is correct. Otherwise setInt(-1) won’t work. If you don’t
want sign extension, then use ‘unsigned’ and not ‘int’ in the template
arguments.
> On Apr 4, 2018, at 14:34, Reid Kleckner via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> I'd argue that bitfield sign extensions are surprising and are usually
a source of bugs. It would be much more explicit and less error prone for the
user to write the sign extension if they want it.
> 
> By extension, it seems good that PointerIntPair doesn't do sign
extension when the type happens to be signed.
> 
> On Wed, Apr 4, 2018 at 9:47 AM David Blaikie via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> I think it'd be reasonable to model this on the same behavior as int to
short to int round-tripping & not to speculate that there might be code
relying on the existing behavior until there's evidence of it.
> 
> I'd suggest changing the behavior & testing to see if anything
breaks - and if nothing does, moving to the behavior rather than supporting
both.
> 
> On Wed, Apr 4, 2018 at 9:27 AM Riyaz Puthiyapurayil
<Riyaz.Puthiyapurayil at synopsys.com <mailto:Riyaz.Puthiyapurayil at
synopsys.com>> wrote:
> Rather than “fixing” it, it might be better to support a separate method
for signed extension. My reasoning is as follows:
> 
>  
> 
> int x = 7;
> 
> llvm::PointerIntPair<double*, 3, int> pip;
> 
> pip.setInt(x);
> 
>  
> 
> There could be code out there that expects pip.getInt() to return 7 and not
-1.
> 
>  
> 
> So if you really want to set a negative and return a negative value, a
separate method setSignedInt and getSignedInt may be OK. Further, sign-extension
would need two shift instructions in X86 as opposed to no-sign extension where
only one ‘and’ with mask is needed for retrieving the int.
> 
>   <>
> From: David Blaikie [mailto:dblaikie at gmail.com <mailto:dblaikie at
gmail.com>]
> Sent: Wednesday, April 4, 2018 7:43 AM
> To: Riyaz Puthiyapurayil <Riyaz.Puthiyapurayil at synopsys.com
<mailto:Riyaz.Puthiyapurayil at synopsys.com>>
> Cc: Florian Hahn <florian.hahn at arm.com <mailto:florian.hahn at
arm.com>>; llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>>; nd <nd at arm.com <mailto:nd at arm.com>>
> Subject: Re: [llvm-dev] llvm::PointerIntPair -- is this by design or a bug?
> 
>  
> 
> I'd suggest someone try fixing this & see if it breaks anything
that can't reasonably be fixed (before we go assuming this is by
design/shouldn't be fixed just because it's the way it is today).
> 
>  
> 
> On Wed, Apr 4, 2018 at 7:16 AM Riyaz Puthiyapurayil via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
> 
> It won't move the sign bit, so negative values won't fit, unless
you have a 3 bit signed type ;)
> 
>  
> 
> Note that if you assign negative values to and then read from a signed
bit-field, you would do sign extension. So 3-bit signed types do exist in C++.
> 
>  
> 
> It begs the question why PointerIntPair supports signed int types if it
always loses the sign. Is it just to avoid signed/unsigned comparison when
comparing the return value of getInt with signed types? Or to use enums that
default to a signed type? In any case, this should be clearly documented if
there is no intention to fix it.
> 
>  
> 
> / Riyaz
> 
> 
> On Apr 4, 2018, at 4:04 AM, Florian Hahn <florian.hahn at arm.com
<mailto:florian.hahn at arm.com>> wrote:
> 
> 
> 
> On 04/04/2018 11:15, David Chisnall wrote:
> 
> 
> On 4 Apr 2018, at 11:01, Florian Hahn via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> 
>  
> 
> Hi,
> 
>  
> 
> On 04/04/2018 05:34, Riyaz Puthiyapurayil via llvm-dev wrote:
> 
> llvm::PointerIntPair<double*, 3, signed> P;
> 
> P.setInt(-4);
> 
> Ideally, the value range for a 3-bit signed integer should be [-4,3]. But
the above call to setInt will fail. Essentially, the signed int field in
PointerIntPair is behaving the same as an 3-bit unsigned field which has the
legal value range of [0,7]. Is this by design? Are negative values not allowed
in PointerIntPair?
> 
>  
> 
> I think that's by design. setInt only allows you to set integer values
that fit into the available bits. It won't move the sign bit, so negative
values won't fit, unless you have a 3 bit signed type ;)
> 
> That doesn’t sound right (for any computer made in the last few decades),
the representation of -3 will be 1111…1111101.  Storing the low bits will yield
101, which is a 3-bit negative three.  When you then sign extend this to any
other signed type, you will get -3 in that representation.  It sounds as if the
signed specialisation of PointerIntPair is simply not doing the sign extension.
> 
> 
> Yep, I meant it looks like it currently does not do a sign extension, it
expects only the available bits to be set, no others. In any case, it is
probably worth documenting the behaviour.
> 
> Cheers,
> Florian
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Ddev&d=DwMFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=mMZWMrEZcvPMLSsEQSah9FOTwza1UudSDkAneN47U9lD3qu6gt3kpnIb4MWV77cM&m=g0Y-dsef9q-C3A1mCQrgZOoqoE7bFtitv8BNSqw9JNY&s=Ka0s2M91ZtQxhGGXhq0Wuf99yfStGshbKPwCryYAqEg&e=>_______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180404/d97c2dd2/attachment.html>

I do agree that sign-extension is the right thing to do. Unlike bit-fields,
llvm::PointerIntPair has asserts checking that the int value is within range. So
if you assign an out of range value, it should fail with an assertion:

llvm::PointerIntPair<SomeType*, 3, int> pip;
pip.setInt(7); // can be made to fail as the valid range
               // of signed 3-bit values is [-4:3]

The above code does not currently fail and instead fails for pip.setInt
arguments with values in [-4:-1] which is actually unexpected and the reason I
started this email thread.

/Riyaz

From: David Zarzycki [mailto:dave at znu.io]
Sent: Wednesday, April 4, 2018 11:47 AM
To: Reid Kleckner <rnk at google.com>
Cc: David Blaikie <dblaikie at gmail.com>; llvm-dev <llvm-dev at
lists.llvm.org>; Riyaz.Puthiyapurayil at synopsys.com; nd <nd at
arm.com>
Subject: Re: [llvm-dev] llvm::PointerIntPair -- is this by design or a bug?

The sign extension is correct. Otherwise setInt(-1) won’t work. If you don’t
want sign extension, then use ‘unsigned’ and not ‘int’ in the template
arguments.


On Apr 4, 2018, at 14:34, Reid Kleckner via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:

I'd argue that bitfield sign extensions are surprising and are usually a
source of bugs. It would be much more explicit and less error prone for the user
to write the sign extension if they want it.

By extension, it seems good that PointerIntPair doesn't do sign extension
when the type happens to be signed.

On Wed, Apr 4, 2018 at 9:47 AM David Blaikie via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:
I think it'd be reasonable to model this on the same behavior as int to
short to int round-tripping & not to speculate that there might be code
relying on the existing behavior until there's evidence of it.

I'd suggest changing the behavior & testing to see if anything breaks -
and if nothing does, moving to the behavior rather than supporting both.
On Wed, Apr 4, 2018 at 9:27 AM Riyaz Puthiyapurayil <Riyaz.Puthiyapurayil at
synopsys.com<mailto:Riyaz.Puthiyapurayil at synopsys.com>> wrote:
Rather than “fixing” it, it might be better to support a separate method for
signed extension. My reasoning is as follows:

int x = 7;
llvm::PointerIntPair<double*, 3, int> pip;
pip.setInt(x);

There could be code out there that expects pip.getInt() to return 7 and not -1.

So if you really want to set a negative and return a negative value, a separate
method setSignedInt and getSignedInt may be OK. Further, sign-extension would
need two shift instructions in X86 as opposed to no-sign extension where only
one ‘and’ with mask is needed for retrieving the int.

From: David Blaikie [mailto:dblaikie at gmail.com<mailto:dblaikie at
gmail.com>]
Sent: Wednesday, April 4, 2018 7:43 AM
To: Riyaz Puthiyapurayil <Riyaz.Puthiyapurayil at
synopsys.com<mailto:Riyaz.Puthiyapurayil at synopsys.com>>
Cc: Florian Hahn <florian.hahn at arm.com<mailto:florian.hahn at
arm.com>>; llvm-dev <llvm-dev at lists.llvm.org<mailto:llvm-dev at
lists.llvm.org>>; nd <nd at arm.com<mailto:nd at arm.com>>
Subject: Re: [llvm-dev] llvm::PointerIntPair -- is this by design or a bug?

I'd suggest someone try fixing this & see if it breaks anything that
can't reasonably be fixed (before we go assuming this is by
design/shouldn't be fixed just because it's the way it is today).

On Wed, Apr 4, 2018 at 7:16 AM Riyaz Puthiyapurayil via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:
It won't move the sign bit, so negative values won't fit, unless you
have a 3 bit signed type ;)

Note that if you assign negative values to and then read from a signed
bit-field, you would do sign extension. So 3-bit signed types do exist in C++.

It begs the question why PointerIntPair supports signed int types if it always
loses the sign. Is it just to avoid signed/unsigned comparison when comparing
the return value of getInt with signed types? Or to use enums that default to a
signed type? In any case, this should be clearly documented if there is no
intention to fix it.

/ Riyaz

On Apr 4, 2018, at 4:04 AM, Florian Hahn <florian.hahn at
arm.com<mailto:florian.hahn at arm.com>> wrote:


On 04/04/2018 11:15, David Chisnall wrote:
On 4 Apr 2018, at 11:01, Florian Hahn via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:

Hi,

On 04/04/2018 05:34, Riyaz Puthiyapurayil via llvm-dev wrote:
llvm::PointerIntPair<double*, 3, signed> P;
P.setInt(-4);
Ideally, the value range for a 3-bit signed integer should be [-4,3]. But the
above call to setInt will fail. Essentially, the signed int field in
PointerIntPair is behaving the same as an 3-bit unsigned field which has the
legal value range of [0,7]. Is this by design? Are negative values not allowed
in PointerIntPair?

I think that's by design. setInt only allows you to set integer values that
fit into the available bits. It won't move the sign bit, so negative values
won't fit, unless you have a 3 bit signed type ;)
That doesn’t sound right (for any computer made in the last few decades), the
representation of -3 will be 1111…1111101.  Storing the low bits will yield 101,
which is a 3-bit negative three.  When you then sign extend this to any other
signed type, you will get -3 in that representation.  It sounds as if the signed
specialisation of PointerIntPair is simply not doing the sign extension.

Yep, I meant it looks like it currently does not do a sign extension, it expects
only the available bits to be set, no others. In any case, it is probably worth
documenting the behaviour.

Cheers,
Florian
_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev<https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Ddev&d=DwMFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=mMZWMrEZcvPMLSsEQSah9FOTwza1UudSDkAneN47U9lD3qu6gt3kpnIb4MWV77cM&m=g0Y-dsef9q-C3A1mCQrgZOoqoE7bFtitv8BNSqw9JNY&s=Ka0s2M91ZtQxhGGXhq0Wuf99yfStGshbKPwCryYAqEg&e=>
_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev<https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Ddev&d=DwMFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=mMZWMrEZcvPMLSsEQSah9FOTwza1UudSDkAneN47U9lD3qu6gt3kpnIb4MWV77cM&m=i1lGzw4GckIVp10bHzumT5KyRBBNaWZswjF74udDSRA&s=75rLj8kH4oLjafQBhbXJdWQTJAko_Joeq-gnb6ZXts0&e=>
_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180405/4571b66c/attachment.html>