Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IPSCCP] Variable not visible at Og: #77901

Merged
merged 5 commits into from
Apr 16, 2024

Conversation

CarlosAlbertoEnciso
Copy link
Member

@CarlosAlbertoEnciso CarlosAlbertoEnciso commented Jan 12, 2024

https://bugs.llvm.org/show_bug.cgi?id=51559
#50901

IPSCCP pass removes the global variable and does not create a constant expression for the initializer value.

Extend test coverage to include:

  • half, bfloat types.
  • checks for undef (int32 and ptr).

There is no support for:

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 12, 2024

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-transforms

Author: Carlos Alberto Enciso (CarlosAlbertoEnciso)

Changes

https://bugs.llvm.org/show_bug.cgi?id=51559
#50901

IPSCCP pass removes the global variable and does not create a constant expression for the initializer value.

Extend test coverage to include:

  • half and bfloat types.
  • checks for undef (int32 and ptr).

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

3 Files Affected:

  • (modified) llvm/lib/Transforms/Utils/Local.cpp (+2-1)
  • (modified) llvm/test/Transforms/SCCP/pr50901.ll (+40-1)
  • (modified) llvm/unittests/Transforms/Utils/LocalTest.cpp (+12-8)
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index b9cad764aaef8b..ecbefca4549caf 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -3594,7 +3594,8 @@ DIExpression *llvm::getExpressionForConstant(DIBuilder &DIB, const Constant &C,
     return createIntegerExpression(C);
 
   auto *FP = dyn_cast<ConstantFP>(&C);
-  if (FP && (Ty.isFloatTy() || Ty.isDoubleTy())) {
+  if (FP &&
+      (Ty.isFloatTy() || Ty.isDoubleTy() || Ty.isHalfTy() || Ty.isBFloatTy())) {
     const APFloat &APF = FP->getValueAPF();
     return DIB.createConstantValueExpression(
         APF.bitcastToAPInt().getZExtValue());
diff --git a/llvm/test/Transforms/SCCP/pr50901.ll b/llvm/test/Transforms/SCCP/pr50901.ll
index 11d6bba6f6a935..d48d67532d88bd 100644
--- a/llvm/test/Transforms/SCCP/pr50901.ll
+++ b/llvm/test/Transforms/SCCP/pr50901.ll
@@ -52,6 +52,16 @@
 ; CHECK:     = !DIGlobalVariableExpression(var: ![[DBG_FLOAT_UNDEF:.+]], expr: !DIExpression())
 ; CHECK-DAG: ![[DBG_FLOAT_UNDEF]]  = distinct !DIGlobalVariable(name: "g_float_undef"
 
+; CHECK: ![[G8:[0-9]+]] = !DIGlobalVariableExpression(var: ![[DBG8:[0-9]+]], expr: !DIExpression(DW_OP_constu, 22136, DW_OP_stack_value))
+; CHECK-DAG: ![[DBG8]] = distinct !DIGlobalVariable(name: "g_88", {{.*}}
+; CHECK: ![[G9:[0-9]+]] = !DIGlobalVariableExpression(var: ![[DBG9:[0-9]+]], expr: !DIExpression(DW_OP_constu, 23726, DW_OP_stack_value))
+; CHECK-DAG: ![[DBG9]] = distinct !DIGlobalVariable(name: "g_99", {{.*}}
+
+; CHECK-DAG: ![[DBGA:[0-9]+]] = distinct !DIGlobalVariable(name: "g_i32_undef"
+; CHECK-DAG: ![[GA:[0-9]+]] = !DIGlobalVariableExpression(var: ![[DBGA]], expr: !DIExpression())
+; CHECK-DAG: ![[DBGB:[0-9]+]] = distinct !DIGlobalVariable(name: "g_ptr_undef"
+; CHECK-DAG: ![[GB:[0-9]+]] = !DIGlobalVariableExpression(var: ![[DBGB]], expr: !DIExpression())
+
 @g_1 = dso_local global i32 -4, align 4, !dbg !0
 @g_2 = dso_local global float 0x4011C28F60000000, align 4, !dbg !8
 @g_3 = dso_local global i8 97, align 1, !dbg !10
@@ -59,6 +69,8 @@
 @g_5 = dso_local global i8 1, align 1, !dbg !16
 @g_6 = dso_local global ptr null, align 8, !dbg !19
 @g_7 = dso_local global ptr null, align 8, !dbg !23
+@g_8 = dso_local global half 0xH4321, align 4, !dbg !86
+@g_9 = dso_local global bfloat 0xR3F80, align 4, !dbg !90
 @_ZL4g_11 = internal global i32 -5, align 4, !dbg !25
 @_ZL4g_22 = internal global float 0x4016333340000000, align 4, !dbg !27
 @_ZL4g_33 = internal global i8 98, align 1, !dbg !29
@@ -67,6 +79,10 @@
 @_ZL4g_66 = internal global ptr null, align 8, !dbg !35
 @_ZL4g_77 = internal global ptr inttoptr (i64 70 to ptr), align 8, !dbg !37
 @g_float_undef = internal global float undef, align 4, !dbg !83
+@_ZL4g_88 = internal global half 0xH5678, align 4, !dbg !88
+@_ZL4g_99 = internal global bfloat 0xR5CAE, align 4, !dbg !92
+@g_i32_undef = internal global i32 undef, align 4, !dbg !95
+@g_ptr_undef = internal global ptr undef, align 8, !dbg !97
 
 define dso_local void @_Z3barv() !dbg !46 {
 entry:
@@ -88,6 +104,15 @@ entry:
   store ptr %6, ptr @g_7, align 8, !dbg !59
   %l = load float, ptr @g_float_undef, align 8, !dbg !59
   store float %l, ptr @g_2, align 8, !dbg !59
+  %7 = load half, ptr @_ZL4g_88, align 4, !dbg !59
+  store half %7, ptr @g_8, align 4, !dbg !59
+  %8 = load bfloat, ptr @_ZL4g_99, align 4, !dbg !59
+  store bfloat %8, ptr @g_9, align 4, !dbg !59
+  %9 = load i32, ptr @g_i32_undef, align 4, !dbg !59
+  store i32 %9, ptr @g_1, align 4, !dbg !59
+  %10 = load ptr, ptr @g_ptr_undef, align 8, !dbg !59
+  store ptr %10, ptr @g_6, align 8, !dbg !59
+
   ret void, !dbg !59
 }
 
@@ -108,7 +133,7 @@ entry:
 !4 = !{!5}
 !5 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !6, size: 64)
 !6 = !DIBasicType(name: "float", size: 32, encoding: DW_ATE_float)
-!7 = !{!0, !8, !10, !13, !16, !19, !23, !25, !27, !29, !31, !33, !35, !37, !83}
+!7 = !{!0, !8, !10, !13, !16, !19, !23, !25, !27, !29, !31, !33, !35, !37, !83, !86, !88, !90, !92, !95, !97}
 !8 = !DIGlobalVariableExpression(var: !9, expr: !DIExpression())
 !9 = distinct !DIGlobalVariable(name: "g_2", scope: !2, file: !3, line: 2, type: !6, isLocal: false, isDefinition: true)
 !10 = !DIGlobalVariableExpression(var: !11, expr: !DIExpression())
@@ -159,3 +184,17 @@ entry:
 !82 = !DILocation(line: 31, column: 1, scope: !77)
 !83 = !DIGlobalVariableExpression(var: !84, expr: !DIExpression())
 !84 = distinct !DIGlobalVariable(name: "g_float_undef", linkageName: "g_float_undef", scope: !2, file: !3, line: 15, type: !6, isLocal: true, isDefinition: true)
+!85 = !DIBasicType(name: "float", size: 16, encoding: DW_ATE_float)
+!86 = !DIGlobalVariableExpression(var: !87, expr: !DIExpression())
+!87 = distinct !DIGlobalVariable(name: "g_8", scope: !2, file: !3, line: 2, type: !85, isLocal: false, isDefinition: true)
+!88 = !DIGlobalVariableExpression(var: !89, expr: !DIExpression())
+!89 = distinct !DIGlobalVariable(name: "g_88", linkageName: "_ZL4g_88", scope: !2, file: !3, line: 10, type: !85, isLocal: true, isDefinition: true)
+!90 = !DIGlobalVariableExpression(var: !91, expr: !DIExpression())
+!91 = distinct !DIGlobalVariable(name: "g_9", scope: !2, file: !3, line: 2, type: !85, isLocal: false, isDefinition: true)
+!92 = !DIGlobalVariableExpression(var: !93, expr: !DIExpression())
+!93 = distinct !DIGlobalVariable(name: "g_99", linkageName: "_ZL4g_99", scope: !2, file: !3, line: 10, type: !85, isLocal: true, isDefinition: true)
+
+!95 = !DIGlobalVariableExpression(var: !96, expr: !DIExpression())
+!96 = distinct !DIGlobalVariable(name: "g_i32_undef", linkageName: "g_i32_undef", scope: !2, file: !3, line: 9, type: !22, isLocal: true, isDefinition: true)
+!97 = !DIGlobalVariableExpression(var: !98, expr: !DIExpression())
+!98 = distinct !DIGlobalVariable(name: "g_ptr_undef", linkageName: "g_ptr_undef", scope: !2, file: !3, line: 14, type: !21, isLocal: true, isDefinition: true)
diff --git a/llvm/unittests/Transforms/Utils/LocalTest.cpp b/llvm/unittests/Transforms/Utils/LocalTest.cpp
index 82257741045754..95f15d99a14eaa 100644
--- a/llvm/unittests/Transforms/Utils/LocalTest.cpp
+++ b/llvm/unittests/Transforms/Utils/LocalTest.cpp
@@ -1241,6 +1241,18 @@ TEST(Local, ExpressionForConstant) {
   EXPECT_NE(Expr, nullptr);
   EXPECT_EQ(Expr->getElement(1), 13841306799765140275U);
 
+  // Half.
+  Type *HalfTy = Type::getHalfTy(Context);
+  Expr = createExpression(ConstantFP::get(HalfTy, 5.55), HalfTy);
+  EXPECT_NE(Expr, nullptr);
+  EXPECT_EQ(Expr->getElement(1), 17805U);
+
+  // BFloat.
+  Type *BFloatTy = Type::getBFloatTy(Context);
+  Expr = createExpression(ConstantFP::get(BFloatTy, -5.55), BFloatTy);
+  EXPECT_NE(Expr, nullptr);
+  EXPECT_EQ(Expr->getElement(1), 49330U);
+
   // Pointer.
   PointerType *PtrTy = PointerType::get(Context, 0);
   Expr = createExpression(ConstantPointerNull::get(PtrTy), PtrTy);
@@ -1258,14 +1270,6 @@ TEST(Local, ExpressionForConstant) {
   EXPECT_EQ(Expr->getElement(1), 5678U);
 
   // Others.
-  Type *HalfTy = Type::getHalfTy(Context);
-  Expr = createExpression(ConstantFP::get(HalfTy, 32), HalfTy);
-  EXPECT_EQ(Expr, nullptr);
-
-  Type *BFloatTy = Type::getBFloatTy(Context);
-  Expr = createExpression(ConstantFP::get(BFloatTy, 32), BFloatTy);
-  EXPECT_EQ(Expr, nullptr);
-
   Type *FP128Ty = Type::getFP128Ty(Context);
   Expr = createExpression(ConstantFP::get(FP128Ty, 32), FP128Ty);
   EXPECT_EQ(Expr, nullptr);

@Endilll Endilll removed their request for review January 12, 2024 10:17
@@ -3594,7 +3594,8 @@ DIExpression *llvm::getExpressionForConstant(DIBuilder &DIB, const Constant &C,
return createIntegerExpression(C);

auto *FP = dyn_cast<ConstantFP>(&C);
if (FP && (Ty.isFloatTy() || Ty.isDoubleTy())) {
if (FP &&
(Ty.isFloatTy() || Ty.isDoubleTy() || Ty.isHalfTy() || Ty.isBFloatTy())) {
Copy link
Contributor

@nikic nikic Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use isFloatingPointTy() here to cover all FP types?

Edit: Well, looking closer, this is bitcasting the float to an integer -- how does debuginfo know to correctly interpret the value? E.g. half and bfloat has the same size, but different semantics.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use isFloatingPointTy() then we should extended the coverage to include X86_FP80TyID and PPC_FP128TyID.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uploaded a patch to support: fp128, x86_fp80, ppc_fp128.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic Very sorry for my late response.

Edit: Well, looking closer, this is bitcasting the float to an integer.

Looking at the documentation for bitcastToAPInt

// This function creates an APInt that is just a bit map of the floating
// point constant as it would appear in memory.  It is not a conversion,
// and treating the result as a normal integer is unlikely to be useful.
APInt IEEEFloat::bitcastToAPInt() const {

The function does not convert the float into an int.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FP value is being bitcast to an int value, so that it can appear in a DWARF expression. To the question about how debug info handles it, my understanding at the moment is that it's up to the consumer. I don't know specifically how different float types of the same size are handled in most debuggers, but there's not much more we can do from this side than provide a bit value and the variable's type AFAICT.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SLTozer Thanks for your additional input on DWARF expression.

Copy link
Contributor

@felipepiovezan felipepiovezan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems reasonable to me, but please wait for a second opinion as well

@CarlosAlbertoEnciso
Copy link
Member Author

CarlosAlbertoEnciso commented Jan 19, 2024

Updated patch to fix assertion detected the BuildKite pre-commit CI:

llvm::APInt::getZExtValue() const:
`getActiveBits() <= 64 && "Too many bits for uint64_t"' failed.

Assertion generated with the introduction of the fp128, x86_fp80 and ppc_fp128 types.

https://bugs.llvm.org/show_bug.cgi?id=51559
llvm#50901

IPSCCP pass removes the global variable and does not
create a constant expression for the initializer value.

Extend test coverage to include:
- half and bfloat types.
- checks for undef (int32 and ptr).
https://bugs.llvm.org/show_bug.cgi?id=51559
llvm#50901

IPSCCP pass removes the global variable and does not
create a constant expression for the initializer value.

- Extend test coverage for all float types:
  Half, bfloat, fp128, x86_fp80, ppc_fp128
https://bugs.llvm.org/show_bug.cgi?id=51559
llvm#50901

IPSCCP pass removes the global variable and does not
create a constant expression for the initializer value.

Fix assertion detected by the BuildKite pre-commit CI:

llvm::APInt::getZExtValue() const:
`getActiveBits() <= 64 && "Too many bits for uint64_t"' failed.
Copy link
Contributor

@SLTozer SLTozer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got an inline comment - the output for >64bit fp values looks broken; I'm fine with just dropping support for those values, though if there's a good fix then that would be ideal.

Comment on lines 65 to 70
; CHECK: ![[G10:[0-9]+]] = !DIGlobalVariableExpression(var: ![[DBG10:[0-9]+]], expr: !DIExpression(DW_OP_constu, 17293822569102704640, DW_OP_stack_value))
; CHECK-DAG: ![[DBG10]] = distinct !DIGlobalVariable(name: "g_1010", {{.*}}
; CHECK: ![[G11:[0-9]+]] = !DIGlobalVariableExpression(var: ![[DBG11:[0-9]+]], expr: !DIExpression(DW_OP_constu, 17293822569102704640, DW_OP_stack_value))
; CHECK-DAG: ![[DBG11]] = distinct !DIGlobalVariable(name: "g_1111", {{.*}}
; CHECK: ![[G12:[0-9]+]] = !DIGlobalVariableExpression(var: ![[DBG12:[0-9]+]], expr: !DIExpression(DW_OP_constu, 14480694097861998592, DW_OP_stack_value))
; CHECK-DAG: ![[DBG12]] = distinct !DIGlobalVariable(name: "g_1212", {{.*}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The values for the >64bit FP values look wrong: the constants for fp128 and ppc_fp128, for example, are equivalent to 0xF000000000000000, just the upper 64 bits of the original value, and for x86_FP80 the value is 0xC8F5C28F5C28F800, just the lower 64 bits of the original value (possibly the implementation gives us the last complete 64-bit word?). If there's not a reliable way to make this work for constant values that don't fit in 64 bits, I think it'd be best to drop them.

NB: Off the top of my head DWARF does have representation for values of arbitrary length, but we don't produce them in LLVM until the point where we emit DWARF; either DW_OP_piece (which we exclusively represent with DW_OP_LLVM_fragment until emission) or DW_OP_implicit_value, which we emit for DBG_VALUEs with constant values and non-complex expressions. The best choice here would probably be to use DW_OP_implicit_value, but I'm not sure how/if we can represent it well in a !DIExpression or whether trying to do so could cause errors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your detailed comment. You are correct.
In the following code:

  if (FP && Ty.isFloatingPointTy()) {
    const APFloat &APF = FP->getValueAPF();
    APInt const &API = APF.bitcastToAPInt();
    if (auto Temp = API.tryZExtValue())
      return DIB.createConstantValueExpression(static_cast<uint64_t>(*Temp));
    else if (auto Temp = API.trySExtValue())
      return DIB.createConstantValueExpression(static_cast<uint64_t>(*Temp));
    return DIB.createConstantValueExpression(*API.getRawData());

tryZExtValue() and trySExtValue() return std::nullopt as the getSignificantBits() is >64
and then getRawData() returns just the upper 64-bits of the original value, which is the value used for the DW_OP_constu.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think dropping support for fp128, ppc_fp128 and x86_fp80 would be a sensible option.
@nikic any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think there's value in supporting those. Just dropping the fallback case where tryZExt/trySExt fail should be fine.

Though I also don't think that trySExt makes sense in the context of a float that has been bitcast to an integer. So I think this should be only tryZExt.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic Thanks.
I have created a PR to record this limitation: #88102

https://bugs.llvm.org/show_bug.cgi?id=51559
llvm#50901

IPSCCP pass removes the global variable and does not
create a constant expression for the initializer value.

Drop support for fp128, ppc_fp128 and x86_fp80 types.
Copy link
Contributor

@SLTozer SLTozer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple comments inline, other than that LGTM.

Comment on lines 3589 to 3590
if (FP &&
(Ty.isBFloatTy() || Ty.isHalfTy() || Ty.isFloatTy() || Ty.isDoubleTy())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (FP &&
(Ty.isBFloatTy() || Ty.isHalfTy() || Ty.isFloatTy() || Ty.isDoubleTy())) {
if (FP && Ty.isFloatingPointTy() && Ty.getScalarSizeInBits() <= 64) {

This could be represented with the above instead, which might make the limitation clearer (i.e. size, not FP semantics) to other users - since I don't think it would make any functional change right now though, this is an optional suggestion.

return DIB.createConstantValueExpression(
APF.bitcastToAPInt().getZExtValue());
APInt const &API = APF.bitcastToAPInt();
if (auto Temp = API.tryZExtValue())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we aren't supporting values >64bits, I think this can revert to using getZExtValue, since tryZExtValue only returns std::nullopt if getActiveBits() <= 64.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

Comment on lines 1269 to 1279
Type *FP128Ty = Type::getFP128Ty(Context);
Expr = createExpression(ConstantFP::get(FP128Ty, 32), FP128Ty);
EXPECT_EQ(Expr, nullptr);

Type *X86_FP80Ty = Type::getX86_FP80Ty(Context);
Expr = createExpression(ConstantFP::get(X86_FP80Ty, 32), X86_FP80Ty);
EXPECT_EQ(Expr, nullptr);

Type *PPC_FP128Ty = Type::getPPC_FP128Ty(Context);
Expr = createExpression(ConstantFP::get(PPC_FP128Ty, 32), PPC_FP128Ty);
EXPECT_EQ(Expr, nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the >64 bit types, we can reinstate these tests for nullptr expressions.

https://bugs.llvm.org/show_bug.cgi?id=51559
llvm#50901

IPSCCP pass removes the global variable and does not
create a constant expression for the initializer value.

Address @SLTozer comments:
- Reinstate the test (FP >= 64) for nullptr expressions.
- Change condition to clarify the FP size restrictions.
- Use 'getZExtValue' instead of 'tryZExtValue'.
@CarlosAlbertoEnciso
Copy link
Member Author

@SLTozer Thanks for your reviews. I will wait for the tests to be completed and then I will commit.

@CarlosAlbertoEnciso CarlosAlbertoEnciso merged commit 66cf995 into llvm:main Apr 16, 2024
4 checks passed
@CarlosAlbertoEnciso CarlosAlbertoEnciso deleted the pr-50901-a branch April 16, 2024 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants