[IPSCCP] Variable not visible at Og. #66745

CarlosAlbertoEnciso · 2023-09-19T07:56:15Z

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.

llvmbot · 2023-09-19T07:57:31Z

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-transforms

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.

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

2 Files Affected:

(modified) llvm/lib/Transforms/IPO/SCCP.cpp (+47)
(added) llvm/test/Transforms/SCCP/pr50901.ll (+184)

diff --git a/llvm/lib/Transforms/IPO/SCCP.cpp b/llvm/lib/Transforms/IPO/SCCP.cpp
index 84f5bbf7039416b..e09769e00148143 100644
--- a/llvm/lib/Transforms/IPO/SCCP.cpp
+++ b/llvm/lib/Transforms/IPO/SCCP.cpp
@@ -22,6 +22,7 @@
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/AttributeMask.h"
 #include "llvm/IR/Constants.h"
+#include "llvm/IR/DIBuilder.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ModRef.h"
@@ -371,6 +372,52 @@ static bool runIPSCCP(
       StoreInst *SI = cast<StoreInst>(GV->user_back());
       SI->eraseFromParent();
     }
+
+    // Try to create a debug constant expression for the glbal variable
+    // initializer value.
+    SmallVector<DIGlobalVariableExpression *, 1> GVEs;
+    GV->getDebugInfo(GVEs);
+    if (GVEs.size() == 1) {
+      DIBuilder DIB(M);
+
+      // Create integer constant expression.
+      auto createIntExpression = [&DIB](const Constant *CV) -> DIExpression * {
+        const APInt &API = dyn_cast<ConstantInt>(CV)->getValue();
+        std::optional<uint64_t> InitIntOpt;
+        if (API.isNonNegative())
+          InitIntOpt = API.tryZExtValue();
+        else if (auto Temp = API.trySExtValue(); Temp.has_value())
+          // Transform a signed optional to unsigned optional.
+          InitIntOpt = (uint64_t)Temp.value();
+        return DIB.createConstantValueExpression(InitIntOpt.value());
+      };
+
+      const Constant *CV = GV->getInitializer();
+      Type *Ty = GV->getValueType();
+      if (Ty->isIntegerTy()) {
+        GVEs[0]->replaceOperandWith(1, createIntExpression(CV));
+      } else if (Ty->isFloatTy() || Ty->isDoubleTy()) {
+        const APFloat &APF = dyn_cast<ConstantFP>(CV)->getValueAPF();
+        DIExpression *NewExpr = DIB.createConstantValueExpression(
+            APF.bitcastToAPInt().getZExtValue());
+        GVEs[0]->replaceOperandWith(1, NewExpr);
+      } else if (Ty->isPointerTy()) {
+        if (isa<ConstantPointerNull>(CV)) {
+          GVEs[0]->replaceOperandWith(1, DIB.createConstantValueExpression(0));
+        } else {
+          if (const ConstantExpr *CE = dyn_cast<ConstantExpr>(CV)) {
+            if (CE->getNumOperands() == 1) {
+              const Value *V = CE->getOperand(0);
+              const Constant *CV = dyn_cast<Constant>(V);
+              if (CV && !isa<GlobalValue>(CV))
+                if (const ConstantInt *CI = dyn_cast<ConstantInt>(CV))
+                  GVEs[0]->replaceOperandWith(1, createIntExpression(CI));
+            }
+          }
+        }
+      }
+    }
+
     MadeChanges = true;
     M.eraseGlobalVariable(GV);
     ++NumGlobalConst;
diff --git a/llvm/test/Transforms/SCCP/pr50901.ll b/llvm/test/Transforms/SCCP/pr50901.ll
new file mode 100644
index 000000000000000..56961d2e32db41c
--- /dev/null
+++ b/llvm/test/Transforms/SCCP/pr50901.ll
@@ -0,0 +1,184 @@
+; RUN: opt -passes=ipsccp -S -o - < %s | FileCheck %s
+
+; Global variables g_11, g_22, g_33, g_44, g_55, g_66 and g_77
+; are not visible in the debugger.
+
+;  1	int       g_1 = -4;
+;  2	float     g_2 = 4.44;
+;  3	char      g_3 = 'a';
+;  4	unsigned  g_4 = 4;
+;  5	bool      g_5 = true;
+;  6	int      *g_6 = nullptr;
+;  7	float    *g_7 = nullptr;
+;  8
+;  9	static int       g_11 = -5;
+; 10	static float     g_22 = 5.55;
+; 11	static char      g_33 = 'b';
+; 12	static unsigned  g_44 = 5;
+; 13	static bool      g_55 = true;
+; 14	static int      *g_66 = nullptr;
+; 15	static float    *g_77 = (float *)(55 + 15);
+; 16
+; 17	void bar() {
+; 18	  g_1 = g_11;
+; 19	  g_2 = g_22;
+; 20	  g_3 = g_33;
+; 21	  g_4 = g_44;
+; 22	  g_5 = g_55;
+; 23	  g_6 = g_66;
+; 24	  g_7 = g_77;
+; 25	}
+; 26
+; 27	int main() {
+; 28	  {
+; 29	    bar();
+; 30	  }
+; 31	}
+
+; CHECK: ![[G1:[0-9]+]] = !DIGlobalVariableExpression(var: ![[DBG1:[0-9]+]], expr: !DIExpression(DW_OP_constu, 18446744073709551611, DW_OP_stack_value))
+; CHECK-DAG: ![[DBG1]] = distinct !DIGlobalVariable(name: "g_11", {{.*}}
+; CHECK: ![[G2:[0-9]+]] = !DIGlobalVariableExpression(var: ![[DBG2:[0-9]+]], expr: !DIExpression(DW_OP_constu, 1085381018, DW_OP_stack_value))
+; CHECK-DAG: ![[DBG2]] = distinct !DIGlobalVariable(name: "g_22", {{.*}}
+; CHECK: ![[G3:[0-9]+]] = !DIGlobalVariableExpression(var: ![[DBG3:[0-9]+]], expr: !DIExpression(DW_OP_constu, 98, DW_OP_stack_value))
+; CHECK-DAG: ![[DBG3]] = distinct !DIGlobalVariable(name: "g_33", {{.*}}
+; CHECK: ![[G4:[0-9]+]] = !DIGlobalVariableExpression(var: ![[DBG4:[0-9]+]], expr: !DIExpression(DW_OP_constu, 5, DW_OP_stack_value))
+; CHECK-DAG: ![[DBG4]] = distinct !DIGlobalVariable(name: "g_44", {{.*}}
+; CHECK: ![[G5:[0-9]+]] = !DIGlobalVariableExpression(var: ![[DBG5:[0-9]+]], expr: !DIExpression(DW_OP_constu, 1, DW_OP_stack_value))
+; CHECK-DAG: ![[DBG5]] = distinct !DIGlobalVariable(name: "g_55", {{.*}}
+; CHECK: ![[G6:[0-9]+]] = !DIGlobalVariableExpression(var: ![[DBG6:[0-9]+]], expr: !DIExpression(DW_OP_constu, 0, DW_OP_stack_value))
+; CHECK-DAG: ![[DBG6]] = distinct !DIGlobalVariable(name: "g_66", {{.*}}
+; CHECK: ![[G7:[0-9]+]] = !DIGlobalVariableExpression(var: ![[DBG7:[0-9]+]], expr: !DIExpression(DW_OP_constu, 70, DW_OP_stack_value))
+; CHECK-DAG: ![[DBG7]] = distinct !DIGlobalVariable(name: "g_77", {{.*}}
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"
+
+@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
+@g_4 = dso_local global i32 4, align 4, !dbg !13
+@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
+@_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
+@_ZL4g_44 = internal global i32 5, align 4, !dbg !31
+@_ZL4g_55 = internal global i8 1, align 1, !dbg !33
+@_ZL4g_66 = internal global ptr null, align 8, !dbg !35
+@_ZL4g_77 = internal global ptr inttoptr (i64 70 to ptr), align 8, !dbg !37
+
+define dso_local void @_Z3barv() !dbg !46 {
+entry:
+  %0 = load i32, ptr @_ZL4g_11, align 4, !dbg !49, !tbaa !50
+  store i32 %0, ptr @g_1, align 4, !dbg !54, !tbaa !50
+  %1 = load float, ptr @_ZL4g_22, align 4, !dbg !55, !tbaa !56
+  store float %1, ptr @g_2, align 4, !dbg !58, !tbaa !56
+  %2 = load i8, ptr @_ZL4g_33, align 1, !dbg !59, !tbaa !60
+  store i8 %2, ptr @g_3, align 1, !dbg !61, !tbaa !60
+  %3 = load i32, ptr @_ZL4g_44, align 4, !dbg !62, !tbaa !50
+  store i32 %3, ptr @g_4, align 4, !dbg !63, !tbaa !50
+  %4 = load i8, ptr @_ZL4g_55, align 1, !dbg !64, !tbaa !65, !range !67, !noundef !68
+  %tobool = trunc i8 %4 to i1, !dbg !64
+  %frombool = zext i1 %tobool to i8, !dbg !69
+  store i8 %frombool, ptr @g_5, align 1, !dbg !69, !tbaa !65
+  %5 = load ptr, ptr @_ZL4g_66, align 8, !dbg !70, !tbaa !71
+  store ptr %5, ptr @g_6, align 8, !dbg !73, !tbaa !71
+  %6 = load ptr, ptr @_ZL4g_77, align 8, !dbg !74, !tbaa !71
+  store ptr %6, ptr @g_7, align 8, !dbg !75, !tbaa !71
+  ret void, !dbg !76
+}
+
+define dso_local noundef i32 @main() !dbg !77 {
+entry:
+  call void @_Z3barv(), !dbg !80
+  ret i32 0, !dbg !82
+}
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!39, !40, !41, !42, !43, !44}
+!llvm.ident = !{!45}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "g_1", scope: !2, file: !3, line: 1, type: !22, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !3, producer: "clang version 18.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, retainedTypes: !4, globals: !7, splitDebugInlining: false, nameTableKind: None)
+!3 = !DIFile(filename: "test.cpp", directory: "")
+!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}
+!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())
+!11 = distinct !DIGlobalVariable(name: "g_3", scope: !2, file: !3, line: 3, type: !12, isLocal: false, isDefinition: true)
+!12 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+!13 = !DIGlobalVariableExpression(var: !14, expr: !DIExpression())
+!14 = distinct !DIGlobalVariable(name: "g_4", scope: !2, file: !3, line: 4, type: !15, isLocal: false, isDefinition: true)
+!15 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
+!16 = !DIGlobalVariableExpression(var: !17, expr: !DIExpression())
+!17 = distinct !DIGlobalVariable(name: "g_5", scope: !2, file: !3, line: 5, type: !18, isLocal: false, isDefinition: true)
+!18 = !DIBasicType(name: "bool", size: 8, encoding: DW_ATE_boolean)
+!19 = !DIGlobalVariableExpression(var: !20, expr: !DIExpression())
+!20 = distinct !DIGlobalVariable(name: "g_6", scope: !2, file: !3, line: 6, type: !21, isLocal: false, isDefinition: true)
+!21 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !22, size: 64)
+!22 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!23 = !DIGlobalVariableExpression(var: !24, expr: !DIExpression())
+!24 = distinct !DIGlobalVariable(name: "g_7", scope: !2, file: !3, line: 7, type: !5, isLocal: false, isDefinition: true)
+!25 = !DIGlobalVariableExpression(var: !26, expr: !DIExpression())
+!26 = distinct !DIGlobalVariable(name: "g_11", linkageName: "_ZL4g_11", scope: !2, file: !3, line: 9, type: !22, isLocal: true, isDefinition: true)
+!27 = !DIGlobalVariableExpression(var: !28, expr: !DIExpression())
+!28 = distinct !DIGlobalVariable(name: "g_22", linkageName: "_ZL4g_22", scope: !2, file: !3, line: 10, type: !6, isLocal: true, isDefinition: true)
+!29 = !DIGlobalVariableExpression(var: !30, expr: !DIExpression())
+!30 = distinct !DIGlobalVariable(name: "g_33", linkageName: "_ZL4g_33", scope: !2, file: !3, line: 11, type: !12, isLocal: true, isDefinition: true)
+!31 = !DIGlobalVariableExpression(var: !32, expr: !DIExpression())
+!32 = distinct !DIGlobalVariable(name: "g_44", linkageName: "_ZL4g_44", scope: !2, file: !3, line: 12, type: !15, isLocal: true, isDefinition: true)
+!33 = !DIGlobalVariableExpression(var: !34, expr: !DIExpression())
+!34 = distinct !DIGlobalVariable(name: "g_55", linkageName: "_ZL4g_55", scope: !2, file: !3, line: 13, type: !18, isLocal: true, isDefinition: true)
+!35 = !DIGlobalVariableExpression(var: !36, expr: !DIExpression())
+!36 = distinct !DIGlobalVariable(name: "g_66", linkageName: "_ZL4g_66", scope: !2, file: !3, line: 14, type: !21, isLocal: true, isDefinition: true)
+!37 = !DIGlobalVariableExpression(var: !38, expr: !DIExpression())
+!38 = distinct !DIGlobalVariable(name: "g_77", linkageName: "_ZL4g_77", scope: !2, file: !3, line: 15, type: !5, isLocal: true, isDefinition: true)
+!39 = !{i32 7, !"Dwarf Version", i32 5}
+!40 = !{i32 2, !"Debug Info Version", i32 3}
+!41 = !{i32 1, !"wchar_size", i32 4}
+!42 = !{i32 8, !"PIC Level", i32 2}
+!43 = !{i32 7, !"PIE Level", i32 2}
+!44 = !{i32 7, !"uwtable", i32 2}
+!45 = !{!"clang version 18.0.0"}
+!46 = distinct !DISubprogram(name: "bar", linkageName: "_Z3barv", scope: !3, file: !3, line: 17, type: !47, scopeLine: 17, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !2)
+!47 = !DISubroutineType(types: !48)
+!48 = !{null}
+!49 = !DILocation(line: 18, column: 9, scope: !46)
+!50 = !{!51, !51, i64 0}
+!51 = !{!"int", !52, i64 0}
+!52 = !{!"omnipotent char", !53, i64 0}
+!53 = !{!"Simple C++ TBAA"}
+!54 = !DILocation(line: 18, column: 7, scope: !46)
+!55 = !DILocation(line: 19, column: 9, scope: !46)
+!56 = !{!57, !57, i64 0}
+!57 = !{!"float", !52, i64 0}
+!58 = !DILocation(line: 19, column: 7, scope: !46)
+!59 = !DILocation(line: 20, column: 9, scope: !46)
+!60 = !{!52, !52, i64 0}
+!61 = !DILocation(line: 20, column: 7, scope: !46)
+!62 = !DILocation(line: 21, column: 9, scope: !46)
+!63 = !DILocation(line: 21, column: 7, scope: !46)
+!64 = !DILocation(line: 22, column: 9, scope: !46)
+!65 = !{!66, !66, i64 0}
+!66 = !{!"bool", !52, i64 0}
+!67 = !{i8 0, i8 2}
+!68 = !{}
+!69 = !DILocation(line: 22, column: 7, scope: !46)
+!70 = !DILocation(line: 23, column: 9, scope: !46)
+!71 = !{!72, !72, i64 0}
+!72 = !{!"any pointer", !52, i64 0}
+!73 = !DILocation(line: 23, column: 7, scope: !46)
+!74 = !DILocation(line: 24, column: 9, scope: !46)
+!75 = !DILocation(line: 24, column: 7, scope: !46)
+!76 = !DILocation(line: 25, column: 1, scope: !46)
+!77 = distinct !DISubprogram(name: "main", scope: !3, file: !3, line: 27, type: !78, scopeLine: 27, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !2)
+!78 = !DISubroutineType(types: !79)
+!79 = !{!22}
+!80 = !DILocation(line: 29, column: 5, scope: !81)
+!81 = distinct !DILexicalBlock(scope: !77, file: !3, line: 28, column: 3)
+!82 = !DILocation(line: 31, column: 1, scope: !77)

jryans

This looks good to me overall, thanks for working on this improvement! 😄

I noticed the PR title ends with a ":", perhaps some further words were lost or the ":" should be removed...?

I recommend waiting a few days for others to take a look as well.

llvm/lib/Transforms/IPO/SCCP.cpp

nikic · 2023-09-19T14:36:15Z

I don't think this chunk of code for converting a Constant into a DIExpression belongs into SCCP.cpp.

llvm/lib/Transforms/IPO/SCCP.cpp

felipepiovezan · 2023-09-19T15:23:47Z

llvm/lib/Transforms/IPO/SCCP.cpp

+      DIBuilder DIB(M);
+
+      // Create integer constant expression.
+      auto createIntExpression = [&DIB](const Constant *CV) -> DIExpression * {


We can also pack this into a neat helper function to simplify the logic

felipepiovezan · 2023-09-19T15:29:56Z

llvm/lib/Transforms/IPO/SCCP.cpp

+          InitIntOpt = API.tryZExtValue();
+        else if (auto Temp = API.trySExtValue(); Temp.has_value())
+          // Transform a signed optional to unsigned optional.
+          InitIntOpt = (uint64_t)Temp.value();


let's avoid C-style casts!

Using instead static_cast<uint64_t>

felipepiovezan · 2023-09-19T15:30:13Z

llvm/lib/Transforms/IPO/SCCP.cpp

+        else if (auto Temp = API.trySExtValue(); Temp.has_value())
+          // Transform a signed optional to unsigned optional.
+          InitIntOpt = (uint64_t)Temp.value();
+        return DIB.createConstantValueExpression(InitIntOpt.value());


This is unconditionally calling std::optional::value, but the control flow above makes it look like we may have a nullopt here. This would be even more evident if we had not used an uninitialized variable here (it would be evident that we are missing an initialization case).

I think we need to account for the possibility of failure here:

DIExpression* createIntExpression(DIB, CV) { const APInt &API = dyn_cast<ConstantInt>(CV)->getValue(); if (API.isNonNegative()) { if(optional<uint64_t> MaybeZExt = API.tryZExtValue()) return DIB.createConstantValueExpression(*MaybeZExt); } else if (optional<uint64_t> MaybeSExt = API.trySExtValue()) return DIB.createConstantValueExpression(* MaybeSExt); return nullptr; };

Oh actually here we have another case where we should use cast<ConstantInt>(CV) instead of dyn_cast

Changed to use cast.

felipepiovezan · 2023-09-19T15:32:27Z

llvm/lib/Transforms/IPO/SCCP.cpp

+      const Constant *CV = GV->getInitializer();
+      Type *Ty = GV->getValueType();
+      if (Ty->isIntegerTy()) {
+        GVEs[0]->replaceOperandWith(1, createIntExpression(CV));


Are we allowed to mutate a metadata like this? IIUC we can only do this if it is uniqued? Or not uniqued? I can never remember exactly

The function replaceOperandWith takes that into consideration.

felipepiovezan · 2023-09-19T15:34:11Z

llvm/lib/Transforms/IPO/SCCP.cpp

+      if (Ty->isIntegerTy()) {
+        GVEs[0]->replaceOperandWith(1, createIntExpression(CV));
+      } else if (Ty->isFloatTy() || Ty->isDoubleTy()) {
+        const APFloat &APF = dyn_cast<ConstantFP>(CV)->getValueAPF();


a dyn_cast that is never checked should be a cast

Changed to cast

llvm/lib/Transforms/IPO/SCCP.cpp

CarlosAlbertoEnciso · 2023-09-20T05:42:34Z

@jryans, @felipepiovezan, @nikic: Thanks for your comments.
I will address them and upload a new patch.

dwblaikie · 2023-09-20T21:40:37Z

llvm/lib/Transforms/IPO/SCCP.cpp

+          InitIntOpt = API.tryZExtValue();
+        else if (auto Temp = API.trySExtValue(); Temp.has_value())
+          // Transform a signed optional to unsigned optional.
+          InitIntOpt = (uint64_t)Temp.value();


Probably also favor * rather than .value() - not sure if we've clarified that in the style guide or not...

Oh, and the if condition could be simpler - no need to use has_value at all:

else if (auto Temp = API.trySExtValue())

Should suffice?

llvm/lib/Transforms/IPO/SCCP.cpp

CarlosAlbertoEnciso · 2023-10-04T08:05:04Z

@jryans, @felipepiovezan, @nikic, @dwblaikie:
Thanks for all your feedback. I have uploaded a new patch that addresses all your points.
I am sorry if there are extra commits on this branch. This is the first update since moving to GitHub.

jryans

Looks good to me, thanks! 😄

Other reviewers had more comments last time around, so probably best to wait for at least one more review before landing.

dwblaikie · 2023-10-04T18:42:31Z

llvm/lib/Transforms/IPO/SCCP.cpp

+      CE->getNumOperands() == 1) {
+    const Value *V = CE->getOperand(0);
+    const Constant *CV = dyn_cast<Constant>(V);
+    if (CV && !isa<GlobalValue>(CV);


I'm not sure this does what's intended (assuming the intent is that the CV && !isa part is meant to be part of the boolean condition of the if as well as the CI init) - these boolean expressions would have no effect, right? (the second part of the if, after the semicolon, would be the only condition?

Yeah, I believe the intent here would be:

if (auto CI = dyn_cast_or_null<ConstantInt>(V)) { ... }

Note that !isa<GlobalValue>(CV) is not necessary: a global value is always a pointer, therefore it cannot be a ConstantInt

Good catch and nice simplification.

dwblaikie · 2023-10-04T18:45:05Z

llvm/lib/Transforms/IPO/SCCP.cpp

+    GVEs[0]->replaceOperandWith(1, DIB.createConstantValueExpression(0));
+    return;
+  }
+  if (const ConstantExpr *CE = dyn_cast<ConstantExpr>(CV);


This is probably problematic too - the initializer part of the if isn't part of the boolean condition - so the CE would be unconditionally dereferenced in the CE->getNumOperands() call - so either the CE init should use cast, not dyn_cast - or the condition needs to be CE && CE->getNumOperands() ... (or nested ifs)

Good catch. Replaced with nested ifs.

This and the other conditions seemed pretty broken, glad they're now fixed - but is there some test coverage that was missed and would've demonstrated these problems/should be added to ensure comprehensive test coverage?

I am happy to add some tests in LocalTest.cpp to ensure we have test coverage for the new function.

felipepiovezan · 2023-10-04T22:16:09Z

llvm/lib/Transforms/IPO/SCCP.cpp

+      InitIntOpt = API.tryZExtValue();
+    else if (auto Temp = API.trySExtValue())
+      // Transform a signed optional to unsigned optional.
+      InitIntOpt = static_cast<uint64_t>(*Temp);


Looking at this again, isn't this the equivalent of always doing a sign extension?
We are basically saying: if this starts with a zero, add zeros; if this starts with a one, add ones.
Unless I am missing something, this code could be:

if (std::optional<sint64_t> InitIntOpt = API.trySExtValue()) return DIB.createConstantValueExpression(static_cast<uint64_t>(*InitIntOpt)) return nullptr;

Good point. I think the original code would be better if we are dealing with APSInt. But for this case, we have APInt.
Changed to

std::optional<int64_t> InitIntOpt = API.trySExtValue(); return InitIntOpt ? DIB.createConstantValueExpression( static_cast<uint64_t>(*InitIntOpt)) : nullptr;

nikic · 2023-10-09T09:01:56Z

I still think that the logic for "convert Constant into DIExpression" should not be part of IPSCCP, but be a helper in some generic location (not sure where, debuginfo folks would know). This seems generically useful (to the point that I'm surprised it does not exist yet).

jryans · 2023-10-09T13:06:30Z

I still think that the logic for "convert Constant into DIExpression" should not be part of IPSCCP, but be a helper in some generic location (not sure where, debuginfo folks would know). This seems generically useful (to the point that I'm surprised it does not exist yet).

I agree, it does seem good to make this available as a shared helper. Perhaps it could go to https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/Local.cpp? There are various shared debug info helpers stashed there already. (At some stage, a separate file just for debug info utils might be a good idea, but that can be separate task.)

CarlosAlbertoEnciso · 2023-10-09T13:06:52Z

@nikic I am happy to move the new logic to some generic location as

DIExpression *convertConstantToExpression(const Constant *CV)
or
DIExpression *getExpressionForConstant(const Constant *CV)

@llvm/pr-subscribers-debuginfo @llvm/issue-subscribers-debuginfo any suggestions?

felipepiovezan

I left two other minor comments, otherwise the patch LGTM!

CarlosAlbertoEnciso · 2023-10-10T07:06:23Z

@jryans, @felipepiovezan: uploaded new patch to reflect your latest comments.
@nikic, @dwblaikie: do you have any additional comments?

nikic

Looks fine, just some nits.

nikic · 2023-10-10T07:20:25Z

llvm/lib/Transforms/Utils/Local.cpp

+  if (const ConstantExpr *CE = dyn_cast<ConstantExpr>(&C)) {
+    if (CE->getNumOperands() == 1) {
+      const Value *V = CE->getOperand(0);
+      if (auto CI = dyn_cast_or_null<ConstantInt>(V)) {


Omit braces

nikic · 2023-10-10T07:21:35Z

llvm/lib/Transforms/Utils/Local.cpp

+    return DIB.createConstantValueExpression(0);
+
+  if (const ConstantExpr *CE = dyn_cast<ConstantExpr>(&C)) {
+    if (CE->getNumOperands() == 1) {


Suggested change

if (CE->getNumOperands() == 1) {

if (CE->getOpcode() == Instruction::IntToPtr) {

I think this is what you're actually trying to handle here? I think this would be clearer than identifying inttoptr as "an expression with one operand that has pointer result and ConstantInt operand".

Very good suggestion.

llvm/test/Transforms/SCCP/pr50901.ll

nikic · 2023-10-10T07:23:03Z

llvm/test/Transforms/SCCP/pr50901.ll

+; CHECK-DAG: ![[DBG7]] = distinct !DIGlobalVariable(name: "g_77", {{.*}}
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"


Data layout and triple shouldn't be necessary. (If you specify a triple, it's either useless or you need to add REQUIRES, otherwise you will break the build.)

Removed datalayout and triple.

djtodoro · 2023-10-10T09:26:41Z

llvm/test/Transforms/SCCP/pr50901.ll

+
+define dso_local void @_Z3barv() !dbg !46 {
+entry:
+  %0 = load i32, ptr @_ZL4g_11, align 4, !dbg !49, !tbaa !50


I believe that you could use only one !dbg for all the instructions in the function and get rid of all the others.

Nice suggestion. Replaced all those !dbgx with just a single one.

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.

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 reviewers comments: - Refactor the new code into a helper function. - Use early returns to reduce nested ifs. - Remove C-style casts. - Add validation to detect posible nullopt.

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 reviewers comments: - Refactor the new code into a helper function. - Use early returns to reduce nested ifs. - Remove C-style casts. - Add validation to detect posible nullopt. - Collapse conditions in a single if. - Remove 'has_value'.

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 reviewers comments: - Refactor the new code into a helper function. - Use early returns to reduce nested ifs. - Remove C-style casts. - Add validation to detect posible nullopt. - Collapse conditions in a single if. - Remove 'has_value'. - Additional if condition simplifications. - Move new logic into a general helper function.

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 reviewers comments: - Refactor the new code into a helper function. - Use early returns to reduce nested ifs. - Remove C-style casts. - Add validation to detect posible nullopt. - Collapse conditions in a single if. - Remove 'has_value'. - Additional if condition simplifications. - Move new logic into a general helper function. - Move new logic to the 'Dbg Intrinsic utilities'" section. - Pass 'Constant' and 'Type' as references. - Remove extra whitespace differences (incorrect clang-format).

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 reviewers comments: - Refactor the new code into a helper function. - Use early returns to reduce nested ifs. - Remove C-style casts. - Add validation to detect posible nullopt. - Collapse conditions in a single if. - Remove 'has_value'. - Additional if condition simplifications. - Move new logic into a general helper function. - Move new logic to the 'Dbg Intrinsic utilities'" section. - Pass 'Constant' and 'Type' as references. - Remove extra whitespace differences (incorrect clang-format). - Use 'Instruction::IntToPtr' with pointer types. - Reduce lit test case. - Add unittests for the new helper function.

CarlosAlbertoEnciso · 2023-10-13T06:17:26Z

Thanks for all your comments.
Updated to address:
@dwblaikie: include unittests (Covering the most common types for constants).
@djtodoro and @nikic: reduce the lit test (Removing redundant !dbg).
@nikic: extra braces and different if condition for pointer types.

dwblaikie · 2023-10-13T19:32:05Z

(nothing further from me, but I'll leave it to other folks more familiar with this to approve)

CarlosAlbertoEnciso · 2023-10-20T05:01:45Z

(nothing further from me, but I'll leave it to other folks more familiar with this to approve)

Thanks

CarlosAlbertoEnciso · 2023-10-20T05:08:00Z

Thanks to all reviewers for your valuable comments.
Any additional points to address, before I merge the changes.

nikic · 2023-10-23T12:36:25Z

I think you can go ahead and merge this. My comments have been addressed, and other people already approved this previously.

Endilll · 2023-12-29T12:34:08Z

Looks like this breaks OpenCL: #50901 (comment)

Check for FP constant instead of checking for floating point types, as Undef/Poison values can have floating point types while not being FPConstants. This fixes a crash introduced by #66745 (f3b20cb).

llvmbot added the llvm:transforms label Sep 19, 2023

CarlosAlbertoEnciso self-assigned this Sep 19, 2023

CarlosAlbertoEnciso requested review from jmorse, adrian-prantl, SLTozer, OCHyams, dwblaikie and pogo59 September 19, 2023 07:59

CarlosAlbertoEnciso added the debuginfo label Sep 19, 2023

jryans approved these changes Sep 19, 2023

View reviewed changes

llvm/lib/Transforms/IPO/SCCP.cpp Outdated Show resolved Hide resolved

felipepiovezan requested changes Sep 19, 2023

View reviewed changes

dwblaikie reviewed Sep 20, 2023

View reviewed changes

CarlosAlbertoEnciso requested a review from a team as a code owner October 4, 2023 07:53

CarlosAlbertoEnciso changed the title ~~[IPSCCP] Variable not visible at Og:~~ [IPSCCP] Variable not visible at Og. Oct 4, 2023

CarlosAlbertoEnciso requested review from felipepiovezan, dwblaikie and jryans October 4, 2023 11:26

jryans approved these changes Oct 4, 2023

View reviewed changes

dwblaikie reviewed Oct 4, 2023

View reviewed changes

ldionne removed the request for review from a team October 4, 2023 19:27

felipepiovezan reviewed Oct 4, 2023

View reviewed changes

CarlosAlbertoEnciso force-pushed the pr-50901 branch from 8e1eb15 to 4b038da Compare October 9, 2023 05:16

CarlosAlbertoEnciso force-pushed the pr-50901 branch from 4b038da to 4df9d34 Compare October 9, 2023 15:28

felipepiovezan approved these changes Oct 9, 2023

View reviewed changes

CarlosAlbertoEnciso force-pushed the pr-50901 branch from 4df9d34 to edac125 Compare October 10, 2023 07:04

nikic reviewed Oct 10, 2023

View reviewed changes

djtodoro reviewed Oct 10, 2023

View reviewed changes

CarlosAlbertoEnciso added 6 commits October 13, 2023 07:09

[IPSCCP] Variable not visible at Og:

07c4f4d

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.

CarlosAlbertoEnciso force-pushed the pr-50901 branch from edac125 to 2b524f1 Compare October 13, 2023 06:11

CarlosAlbertoEnciso requested review from nikic, jryans, felipepiovezan and djtodoro October 13, 2023 06:17

CarlosAlbertoEnciso merged commit f3b20cb into llvm:main Oct 24, 2023
3 checks passed

Endilll mentioned this pull request Dec 29, 2023

Variable not visible at Og #50901

Closed

	if (CE->getNumOperands() == 1) {
	if (CE->getOpcode() == Instruction::IntToPtr) {

[IPSCCP] Variable not visible at Og. #66745

[IPSCCP] Variable not visible at Og. #66745

Conversation

CarlosAlbertoEnciso commented Sep 19, 2023

llvmbot commented Sep 19, 2023 • edited

jryans left a comment

Choose a reason for hiding this comment

nikic commented Sep 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipepiovezan Sep 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarlosAlbertoEnciso commented Sep 20, 2023

Choose a reason for hiding this comment

CarlosAlbertoEnciso commented Oct 4, 2023

jryans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic commented Oct 9, 2023

jryans commented Oct 9, 2023

CarlosAlbertoEnciso commented Oct 9, 2023

felipepiovezan left a comment

Choose a reason for hiding this comment

CarlosAlbertoEnciso commented Oct 10, 2023

nikic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarlosAlbertoEnciso commented Oct 13, 2023

dwblaikie commented Oct 13, 2023

CarlosAlbertoEnciso commented Oct 20, 2023

CarlosAlbertoEnciso commented Oct 20, 2023

nikic commented Oct 23, 2023

Endilll commented Dec 29, 2023

llvmbot commented Sep 19, 2023 •

edited

felipepiovezan Sep 19, 2023 •

edited