[Regression] Infinite loop in SoftenFloatOperand

llvmbot commented 6 years ago


Bugzilla Link	34866
Resolution	FIXED
Resolved on	Oct 12, 2017 10:53
Version	5.0
OS	Windows NT
Reporter	LLVM Bugzilla Contributor
CC	@fhahn,@RKSimon,@rotateright,@stephenhines

Extended Description

The following code no longer compiles: $ cat test.c

typedef unsigned __int128 u128_t; typedef __float128 f128_t;

typedef union { f128_t f; u128_t i; } union128_t;

int test(volatile f128_t start_X_ptr, volatile f128_t start_Y_ptr) { for (unsigned iter_count = 1; iter_count <= 2; iter_count++) { union128_t read_valX = { .f = start_X_ptr }; union128_t read_valY = { .f = start_Y_ptr };

    u128_t expected_val = ((iter_count - 1) * read_valY.i);
    if (read_valX.i != expected_val)
    {
        return -1;
    }

    union128_t write_valX = { .i = (iter_count * (read_valX.i + 1)) };
    union128_t write_valY = { .i = (read_valX.i + 1) };

    *start_X_ptr = write_valX.f;
    *start_Y_ptr = write_valY.f;
}
return 0;

}

because of an infinite loop in SoftenFloatOperand.

$ clang -O3 -c test.c -mllvm -debug-only=legalize-types .... Soften float operand 0: t33: v16i8 = bitcast t20

Soften float operand 0: t33: v16i8 = bitcast t20

Works with LLVM4.0.1: https://godbolt.org/g/W8cULV

I suspect this was introduced in r254653 and got exposed sometime between 03/14/2017 and 03/17/2017.

llvmbot commented 2 years ago

mentioned in issue llvm/llvm-bugzilla-archive#34918

llvmbot commented 6 years ago

Marking it resolved and remove it as blocking 5.0.1 release. llvm/llvm-bugzilla-archive#34918 now tracks merging r315485 into 5.0.1

rotateright commented 6 years ago

It would be nice if we can add asserts too, but that should not block this bug from being marked as resolved for 5.0.1 release IMHO.

Ok, I'll let you decide. I don't know enough about softfloat to make changes there, so I don't have an immediate plan to add any asserts myself.

llvmbot commented 6 years ago

It would be nice if we can add asserts too, but that should not block this bug from being marked as resolved for 5.0.1 release IMHO.

rotateright commented 6 years ago

The inf-loop should be fixed after: https://reviews.llvm.org/rL315485

Do we prefer to mark the report as 'resolved' or leave it open for tracking the adding of more asserts and/or blocking the 5.0.1 release?

llvmbot commented 6 years ago

Yes, I like more assertions. Many are already in LegalizeTypes.cpp and LegalizeFloatTypes.cpp. My knowledge of these modules is also limited. The loops in DAGTypeLegalizer::run are really complicate. I am not sure where to add more assertions.

llvmbot commented 6 years ago

AFAIK, SoftenFloat* function assume that f128 values have f128 type or are cast into i128 type. So optimizations like D31156 could break this assumption.

I think D38771 is right to skip f128 type. Would it be good idea to add an assert to SoftenFloat* functions so that future optimizations won't break this assumption?

llvmbot commented 6 years ago

__float128 or MVT::f128 should be IEEE 128-bit floating point format. How such 128-bit values are stored/passed in X86 registers depends on available registers, and hence you see code depending on hasMMX().

Since floating point operations are slow, many libraries cast float128 to int128 and manipulate the bits as in test.c, with a union128_t. The clang compiler should not abort, but translate the cast and follow up __int128 operations as expected in C/C++.

SoftenFloat function are fragile. They convert float128 or int128 instructions into int128 or int64 and pass them to library functions. New code patterns generated by optimization could break SoftenFloat.

AFAIK, SoftenFloat* function assume that f128 values have f128 type or are cast into i128 type. So optimizations like D31156 could break this assumption.

I think D38771 is right to skip f128 type.

rotateright commented 6 years ago

Patch posted for review: https://reviews.llvm.org/D38771

I'd be happier if someone could explain what is happening / expected for this code. :)

RKSimon commented 6 years ago

Reduced testcase:

; ModuleID = 'bugpoint-reduced-simplified.bc' source_filename = "bugpoint-output-0526374.bc" target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64--linux"

; Function Attrs: norecurse nounwind uwtable define i1 @Z4testPVgS0() local_unnamed_addr #0 { entry: %tmp = bitcast fp128 0xL00000000000000000000000000000000 to i128 %cmp3.1 = icmp eq i128 %tmp, undef ret i1 %cmp3.1 }

attributes #0 = { norecurse nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="btver2" "target-features"="+aes,+avx,+bmi,+cx16,+f16c,+fxsr,+lzcnt,+mmx,+movbe,+pclmul,+popcnt,+prfchw,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+sse4a,+ssse3,+x87,+xsave,+xsaveopt" "unsafe-fp-math"="false" "use-soft-float"="false" }

llvmbot commented 6 years ago

I have reproduced this problem with llvm r298376, but not with r298375. r298376 was introduced in https://reviews.llvm.org/D31156 Could D31156 author or reviewer take a look?

llvmbot commented 6 years ago

I can reproduce the problem. Will take a look.

llvmbot commented 6 years ago

Correction:

I suspect this was introduced in r254653 and got exposed sometime between 03/17/2017 and 03/23/2017.

llvm / llvm-project

[Regression] Infinite loop in SoftenFloatOperand #34214

Extended Description