[InstCombine] Avoid crash on aggregate types in SimplifyDemandedUseFPClass

[MacDue]

The disables folding for FP aggregates that are not poison/posZero types, which is currently not supported. Note: To fully handle this aggregates would also likely require teaching computeKnownFPClass() to handle array and struct constants (which does not seem implemented outside of zero init).

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Benjamin Maxwell (MacDue)

Changes

The disables folding to FP aggregates that are not poison types, which is currently not supported. Note: To fully handle this case would also likely require teaching computeKnownFPClass() to handle array and struct constants (which does not seem implemented outside of zero init).

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp (+13)
• (modified) llvm/test/Transforms/InstCombine/simplify-demanded-fpclass.ll (+29)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
index ee6b60f7f70d68..cb8c0dff7ba7dd 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
@@ -1919,6 +1919,19 @@ Value *InstCombinerImpl::SimplifyDemandedVectorElts(Value *V,
 /// For floating-point classes that resolve to a single bit pattern, return that
 /// value.
 static Constant *getFPClassConstant(Type *Ty, FPClassTest Mask) {
+  // TODO: Support aggregate types that are allowed by FPMathOperator.
+  switch (Mask) {
+    case fcPosZero:
+    case fcNegZero:
+    case fcPosInf:
+    case fcNegInf:
+      if (Ty->isAggregateType())
+        return nullptr;
+      break;
+    default:
+      break;
+  }
+
   switch (Mask) {
   case fcPosZero:
     return ConstantFP::getZero(Ty);
diff --git a/llvm/test/Transforms/InstCombine/simplify-demanded-fpclass.ll b/llvm/test/Transforms/InstCombine/simplify-demanded-fpclass.ll
index 403f3bacf34d89..c03c31f4f41ba7 100644
--- a/llvm/test/Transforms/InstCombine/simplify-demanded-fpclass.ll
+++ b/llvm/test/Transforms/InstCombine/simplify-demanded-fpclass.ll
@@ -91,6 +91,35 @@ define nofpclass(inf) float @ret_nofpclass_inf__ninf() {
   ret float 0xFFF0000000000000
 }
 
+; Basic aggregate tests to ensure this does not crash.
+define nofpclass(nan) { float } @ret_nofpclass_struct_ty() {
+; CHECK-LABEL: define nofpclass(nan) { float } @ret_nofpclass_struct_ty() {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    ret { float } zeroinitializer
+;
+entry:
+  ret { float } zeroinitializer
+}
+
+
+define nofpclass(nan) [ 5 x float ] @ret_nofpclass_array_ty() {
+; CHECK-LABEL: define nofpclass(nan) [5 x float] @ret_nofpclass_array_ty() {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    ret [5 x float] zeroinitializer
+;
+entry:
+  ret [ 5 x float ] zeroinitializer
+}
+
+define nofpclass(nan) [ 2 x [ 5 x float ]] @ret_nofpclass_nested_array_ty() {
+; CHECK-LABEL: define nofpclass(nan) [2 x [5 x float]] @ret_nofpclass_nested_array_ty() {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    ret [2 x [5 x float]] zeroinitializer
+;
+entry:
+  ret [ 2 x [ 5 x float ]] zeroinitializer
+}
+
 ; Negative test, do nothing
 define nofpclass(inf) float @ret_nofpclass_inf__select_nofpclass_inf_lhs(i1 %cond, float nofpclass(inf) %x, float %y) {
 ; CHECK-LABEL: define nofpclass(inf) float @ret_nofpclass_inf__select_nofpclass_inf_lhs

[MacDue]

@mikaelholmen could you test if this resolves the issue you reported? (#110506 (comment)) 🙂

[arsenm]

It wasn't obvious to me why these pre-cases. Merge the switches?

[MacDue]

Refactored now, but I just didn't want to repeat the isAggregateType() check for every case.

[arsenm]

These cases aren't tested

[MacDue]

I've put cases that (I think) should hit these, however, computeKnownFPClass() only handles ConstantAggregateZero (and that case is now fixed), so I'm not sure it's possible to get fcNegZero, fcPosInf etc for a constant aggregate.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[nikic]

It's okay to do this unconditionally, without repeating the switch.

[mikaelholmen]

@mikaelholmen could you test if this resolves the issue you reported? (#110506 (comment)) 🙂

Indeed it does. Thank you!