[ADT] Fix a bug in PackedVector::setValue for signed types

[kazutakahirata]

Without this patch, we forget to update the sign bit. When we assign:

Vec[0] = -1;

the sign bit is correctly set to 1. Overwriting the same element:

Vec[0] = 1;

does not update the sign bit, leaving the 4-bit as 0b1001, which reads
-2 according to PackedVector's encoding. (It does not use two's
complement.)

This patch fixes the bug by clearing the sign bit when we are
assigning a non-negative value.

[llvmbot]

@llvm/pr-subscribers-llvm-adt

Author: Kazu Hirata (kazutakahirata)

Changes

Without this patch, we forget to update the sign bit. When we assign:

Vec[0] = -1;

the sign bit is correctly set to 1. Overwriting the same element:

Vec[0] = 1;

does not update the sign bit, leaving the 4-bit as 0b1001, which reads
-2 according to PackedVector's encoding. (It does not use two's
complement.)

This patch fixes the bug by clearing the sign bit when we are
assigning a non-negative value.

---

Full diff: https://github.com/llvm/llvm-project/pull/159239.diff

2 Files Affected:

• (modified) llvm/include/llvm/ADT/PackedVector.h (+2)
• (modified) llvm/unittests/ADT/PackedVectorTest.cpp (+8)

diff --git a/llvm/include/llvm/ADT/PackedVector.h b/llvm/include/llvm/ADT/PackedVector.h
index 1146cc4bd6d23..4e31d3f098d44 100644
--- a/llvm/include/llvm/ADT/PackedVector.h
+++ b/llvm/include/llvm/ADT/PackedVector.h
@@ -58,6 +58,8 @@ class PackedVectorBase<T, BitNum, BitVectorTy, true> {
     if (val < 0) {
       val = ~val;
       Bits.set((Idx * BitNum) + BitNum - 1);
+    } else {
+      Bits.reset((Idx * BitNum) + BitNum - 1);
     }
     assert((val >> (BitNum-1)) == 0 && "value is too big");
     for (unsigned i = 0; i != BitNum-1; ++i)
diff --git a/llvm/unittests/ADT/PackedVectorTest.cpp b/llvm/unittests/ADT/PackedVectorTest.cpp
index 30fc7c0b6d07f..df2cbf0e7f0f8 100644
--- a/llvm/unittests/ADT/PackedVectorTest.cpp
+++ b/llvm/unittests/ADT/PackedVectorTest.cpp
@@ -71,6 +71,14 @@ TEST(PackedVectorTest, RawBitsSize) {
   EXPECT_EQ(12u, Vec.raw_bits().size());
 }
 
+TEST(PackedVectorTest, SignedValueOverwrite) {
+  PackedVector<signed, 4> Vec(1);
+  Vec[0] = -1;
+  EXPECT_EQ(-1, Vec[0]);
+  Vec[0] = 1;
+  EXPECT_EQ(1, Vec[0]);
+}
+
 #ifdef EXPECT_DEBUG_DEATH
 
 TEST(PackedVectorTest, UnsignedValues) {

[arsenm]

I'd expect signed types to be disallowed?

[kazutakahirata]

The author seems to mean to support signed types as far as I can tell from PackedVectorBase.

template <typename T, unsigned BitNum, typename BitVectorTy, bool isSigned>
class PackedVectorBase;

template <typename T, unsigned BitNum, typename BitVectorTy>
class PackedVectorBase<T, BitNum, BitVectorTy, false> {
  :
  :
};

template <typename T, unsigned BitNum, typename BitVectorTy>
class PackedVectorBase<T, BitNum, BitVectorTy, true> {
  :
  :
};

[kuhar]

I'd think that in order to support signed types, we have to take care of sign extension and that merely manipulating the MSB is not enough? I haven't looked at the logic carefully, but this sounds fishy...

[kazutakahirata]

I'd think that in order to support signed types, we have to take care of sign extension and that merely manipulating the MSB is not enough? I haven't looked at the logic carefully, but this sounds fishy...

getValue handles sign extension with ~ in the signed case:

  static T getValue(const BitVectorTy &Bits, unsigned Idx) {
    T val = T();
    for (unsigned i = 0; i != BitNum-1; ++i)
      val = T(val | ((Bits[(Idx * BitNum) + i] ? 1UL : 0UL) << i));
    if (Bits[(Idx * BitNum) + BitNum - 1])  //  <---- Look
      val = ~val;                           //  <---- Look
    return val;
  }

That is, if the sign bit is set, we flip the whole value with val = ~val. Again, we do not use two's complement in PackedVector.

[kazutakahirata]

Friendly ping. Thanks!