Bug in InsertElement constant propagation?

Hi,

When I run opt on the following LLVM IR:

define i32 @foo() {

bb0:

%0 = bitcast i32 2139171423 to float

%1 = insertelement <1 x float> undef, float %0, i32 0

%2 = extractelement <1 x float> %1, i32 0

%3 = bitcast float %2 to i32

ret i32 %3

}

It generates:

define i32 @foo() {

bb0:

ret i32 2143365727

}

While tracking the value I see that the floating point value is changed while folding insertElement into a constant expression.

In Constant.cpp line 902, we convert the APFloat into float then convert it back when creating the ConstantDataArray. This causes the float value to be changed. I’m not entirely positive why the float value returned by converToFloat has a different memory representation than the original int value, there must be a C++ rule that I’m missing.

d

if (ConstantFP *CFP = dyn_cast(C)) {

if (CFP->getType()->isFloatTy()) {

SmallVector<float, 16> Elts;

for (unsigned i = 0, e = V.size(); i != e; ++i)

if (ConstantFP *CFP = dyn_cast(V[i]))

Elts.push_back(CFP->getValueAPF().convertToFloat());

else

break;

if (Elts.size() == V.size())

return ConstantDataArray::get(C->getContext(), Elts);

}

Anyone knows what is the problem here?

Cheers,

Thomas

Hi,

When I run opt on the following LLVM IR:

define i32 @foo() {

bb0:

   %0 = bitcast i32 2139171423 to float

   %1 = insertelement <1 x float> undef, float %0, i32 0

   %2 = extractelement <1 x float> %1, i32 0

   %3 = bitcast float %2 to i32

   ret i32 %3

}

->

It generates:

define i32 @foo() {

bb0:

   ret i32 2143365727

}

While tracking the value I see that the floating point value is changed
while folding insertElement into a constant expression.

In Constant.cpp line 902, we convert the APFloat into float then convert
it back when creating the ConstantDataArray. This causes the float value
to be changed. I’m not entirely positive why the float value returned by
converToFloat has a different memory representation than the original
int value, there must be a C++ rule that I’m missing.

d

     if (ConstantFP *CFP = dyn_cast<ConstantFP>(C)) {

       if (CFP->getType()->isFloatTy()) {

         SmallVector<float, 16> Elts;

         for (unsigned i = 0, e = V.size(); i != e; ++i)

           if (ConstantFP *CFP = dyn_cast<ConstantFP>(V[i]))

             Elts.push_back(CFP->getValueAPF().convertToFloat());

           else

             break;

         if (Elts.size() == V.size())

           return ConstantDataArray::get(C->getContext(), Elts);

       }

Anyone knows what is the problem here?

Both bit patterns are representations of NaN, which IIRC, is not required to be preserved when copying a float.

Jon

Ha here is what I was missing. Thanks Jon. It still seems to me that the transformation of LLVM IR is invalid is that right? I assume we shouldn't be converting APFloat to float in order to avoid such problems?

Ha here is what I was missing. Thanks Jon. It still seems to me that the transformation of LLVM IR is invalid is that right?

I don't know if IR is required to preserve NaN bit patterns, but ISTM that it would be better if it did.

I assume we shouldn't be converting APFloat to float in order to avoid such problems?

Yes, I think that's the correct fix.

Jon

I don't see a way to create a ConstantDataVector from Constant or form APFloat though. Did I oversee that?
Is the solution to had a new get function in ConstantDataVector to allow that? Any hint on what would be the right fix otherwise?

Thomas

I'm not sure what the right fix is. I don't see a way to get the float out of APFloat without a copy happening. The usual trick to copy a float somewhere else but preserve the NaN bits is to use memcpy.

Jon

Does anybody else have an opinion on this issue? I'm planning to submit a patch which would add a new get method for ConstantDataVector taking an ArrayRef<Constant*> and use that in the few places in constant propagation where convertToFloat is used.

Let me know if you think there is a more obvious way to do it.

Right now the only way to create a ConstantDataVector are those method:

  /// get() constructors - Return a constant with vector type with an element
  /// count and element type matching the ArrayRef passed in. Note that this
  /// can return a ConstantAggregateZero object.
  static Constant *get(LLVMContext &Context, ArrayRef<uint8_t> Elts);
  static Constant *get(LLVMContext &Context, ArrayRef<uint16_t> Elts);
  static Constant *get(LLVMContext &Context, ArrayRef<uint32_t> Elts);
  static Constant *get(LLVMContext &Context, ArrayRef<uint64_t> Elts);
  static Constant *get(LLVMContext &Context, ArrayRef<float> Elts);
  static Constant *get(LLVMContext &Context, ArrayRef<double> Elts);

  /// getSplat - Return a ConstantVector with the specified constant in each
  /// element. The specified constant has to be a of a compatible type (i8/i16/
  /// i32/i64/float/double) and must be a ConstantFP or ConstantInt.
  static Constant *getSplat(unsigned NumElts, Constant *Elt);

Cheers,
Thomas

Raoux, Thomas F wrote:

Does anybody else have an opinion on this issue? I'm planning to submit a patch which would add a new get method for ConstantDataVector taking an ArrayRef<Constant*> and use that in the few places in constant propagation where convertToFloat is used.

Let me know if you think there is a more obvious way to do it.

Right now the only way to create a ConstantDataVector are those method:

   /// get() constructors - Return a constant with vector type with an element
   /// count and element type matching the ArrayRef passed in. Note that this
   /// can return a ConstantAggregateZero object.
   static Constant *get(LLVMContext&Context, ArrayRef<uint8_t> Elts);
   static Constant *get(LLVMContext&Context, ArrayRef<uint16_t> Elts);
   static Constant *get(LLVMContext&Context, ArrayRef<uint32_t> Elts);
   static Constant *get(LLVMContext&Context, ArrayRef<uint64_t> Elts);
   static Constant *get(LLVMContext&Context, ArrayRef<float> Elts);
   static Constant *get(LLVMContext&Context, ArrayRef<double> Elts);

   /// getSplat - Return a ConstantVector with the specified constant in each
   /// element. The specified constant has to be a of a compatible type (i8/i16/
   /// i32/i64/float/double) and must be a ConstantFP or ConstantInt.
   static Constant *getSplat(unsigned NumElts, Constant *Elt);

I haven't followed the whole thread through, but my first question is whether APFloat::bitcastToAPInt helps? Secondly, what is your constructor doing with this Constant*, the point of ConstantDataFoo is to avoid the construction of the Constant and its internal Use objects -- especially to avoid the Use objects. It could make sense if your ctor takes the Constant merely as an input to rip the data out of, but not if it tries to hold the Constant* in any way.

Nick

So right now the constant is converted to a float using convertToFloat (which uses bitCasttoAPInt) so the original float value is fine but then when it gets passed around it may change.

if (ConstantFP *CFP = dyn_cast<ConstantFP>(C)) {
      if (CFP->getType()->isFloatTy()) {
        SmallVector<float, 16> Elts;
        for (unsigned i = 0, e = V.size(); i != e; ++i)
          if (ConstantFP *CFP = dyn_cast<ConstantFP>(V[i]))
            Elts.push_back(CFP->getValueAPF().convertToFloat()); --> Here the copy of float may change the raw value
          else
            break;
        if (Elts.size() == V.size())
          return ConstantDataArray::get(C->getContext(), Elts);
      }

Yes what I have in mind is to provide a constructor taking Constant* as parameter which would store the data in the raw format as it is intended for ConstantDataVector/ConstantDataArray.

Thomas