[PATCH] D91806: [SVE] Remove warning from debug info on scalable vector.

[Joe Ellis via Phabricator via cfe-commits]

joechrisellis updated this revision to Diff 308964.
joechrisellis added a comment.

Address @sdesmalen's comments.

- Update commit message to better reflect the changes in this patch.
- Rename `debug-declare-no-warnings-on-scalable-vectors.ll` to 
`dbg-info-scalable-typesize-warning.ll`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91806/new/

https://reviews.llvm.org/D91806

Files:
  llvm/lib/Transforms/Utils/Debugify.cpp
  llvm/lib/Transforms/Utils/Local.cpp
  llvm/test/Transforms/InstCombine/dbg-info-scalable-typesize-warning.ll

Index: llvm/test/Transforms/InstCombine/dbg-info-scalable-typesize-warning.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/dbg-info-scalable-typesize-warning.ll
@@ -0,0 +1,36 @@
+; RUN: opt -mtriple aarch64-gnu-linux -mattr=+sve -instcombine -S < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; This test is defending against a TypeSize warning raised in the method
+; `valueCoversEntireFragment` in Local.cpp because of an implicit cast from
+; `TypeSize` to `uint64_t`. This particular TypeSize warning only occurred when
+; debug info was available.
+
+; If this check fails please read
+; clang/test/CodeGen/aarch64-sve-intrinsics/README for instructions on
+; how to resolve it.
+
+; WARN-NOT: warning: {{.*}}TypeSize is not scalable
+
+; CHECK-LABEL: @debug_local_scalable(
+define  @debug_local_scalable( %tostore) {
+  %vx = alloca , align 16
+  call void @llvm.dbg.declare(metadata * %vx, metadata !3, metadata !DIExpression()), !dbg !5
+  store  %tostore, * %vx, align 16
+  %ret = call  @f(* %vx)
+  ret  %ret
+}
+
+declare  @f(*)
+
+; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
+!1 = !DIFile(filename: "/tmp/test.c", directory: "/tmp/")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !DILocalVariable(scope: !4)
+!4 = distinct !DISubprogram(unit: !0)
+!5 = !DILocation(scope: !4)
Index: llvm/lib/Transforms/Utils/Local.cpp
===
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -1368,16 +1368,22 @@
 /// least n bits.
 static bool valueCoversEntireFragment(Type *ValTy, DbgVariableIntrinsic *DII) {
   const DataLayout  = DII->getModule()->getDataLayout();
-  uint64_t ValueSize = DL.getTypeAllocSizeInBits(ValTy);
-  if (auto FragmentSize = DII->getFragmentSizeInBits())
-return ValueSize >= *FragmentSize;
+  TypeSize ValueSize = DL.getTypeAllocSizeInBits(ValTy);
+  if (Optional FragmentSize = DII->getFragmentSizeInBits()) {
+assert(!ValueSize.isScalable() &&
+   "Fragments don't work on scalable types.");
+return ValueSize.getFixedSize() >= *FragmentSize;
+  }
   // We can't always calculate the size of the DI variable (e.g. if it is a
   // VLA). Try to use the size of the alloca that the dbg intrinsic describes
   // intead.
   if (DII->isAddressOfVariable())
 if (auto *AI = dyn_cast_or_null(DII->getVariableLocation()))
-  if (auto FragmentSize = AI->getAllocationSizeInBits(DL))
-return ValueSize >= *FragmentSize;
+  if (Optional FragmentSize = AI->getAllocationSizeInBits(DL)) {
+assert(ValueSize.isScalable() == FragmentSize->isScalable() &&
+   "Both sizes should agree on the scalable flag.");
+return TypeSize::isKnownGE(ValueSize, *FragmentSize);
+  }
   // Could not determine size of variable. Conservatively return false.
   return false;
 }
Index: llvm/lib/Transforms/Utils/Debugify.cpp
===
--- llvm/lib/Transforms/Utils/Debugify.cpp
+++ llvm/lib/Transforms/Utils/Debugify.cpp
@@ -44,8 +44,9 @@
 
 raw_ostream () { return Quiet ? nulls() : errs(); }
 
-uint64_t getAllocSizeInBits(Module , Type *Ty) {
-  return Ty->isSized() ? M.getDataLayout().getTypeAllocSizeInBits(Ty) : 0;
+TypeSize getAllocSizeInBits(Module , Type *Ty) {
+  return Ty->isSized() ? M.getDataLayout().getTypeAllocSizeInBits(Ty)
+   : TypeSize::getFixed(0);
 }
 
 bool isFunctionSkipped(Function ) {
@@ -276,7 +277,7 @@
 return false;
 
   Type *Ty = V->getType();
-  uint64_t ValueOperandSize = getAllocSizeInBits(M, Ty);
+  TypeSize ValueOperandSize = getAllocSizeInBits(M, Ty);
   Optional DbgVarSize = DVI->getFragmentSizeInBits();
   if (!ValueOperandSize || !DbgVarSize)
 return false;
@@ -285,7 +286,7 @@
   if (Ty->isIntegerTy()) {
 auto Signedness = DVI->getVariable()->getSignedness();
 if (Signedness && *Signedness == DIBasicType::Signedness::Signed)
-  HasBadSize = ValueOperandSize < *DbgVarSize;
+  HasBadSize = ValueOperandSize.getFixedSize() < *DbgVarSize;
   } else {
 HasBadSize = ValueOperandSize 

[PATCH] D91806: [SVE] Remove warning from debug info on scalable vector.

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added a comment.

Please rename `debug-declare-no-warnings-on-scalable-vectors.ll` to something 
different, because these 'warnings' are only temporary and will be replaced by 
errors in the future. Having 'no warnings' in the name of the test name seems 
wrong from that perspective.

In D91806#2413692 , @fpetrogalli wrote:

>> This patch needs to be retitled to what this is actually doing: changing the 
>> getTypeAllocationSizeInBits and getFragmentSizeInBits to return a TypeSize 
>> instead of unsigned.
>
> Given that the only change resulting from this patch is removing the warning, 
> I think it makes sense to keep the summary of the patch as it is.

Given that the generated warnings are temporary and only a side-effect of the 
implicit conversion 'hack', and thus not a feature or design choice, I still 
think the patch needs to be retitled to better phrase what it does, i.e. change 
getTypeAllocationSizeInBits to return TypeSize instead of unsigned.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91806/new/

https://reviews.llvm.org/D91806

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91806: [SVE] Remove warning from debug info on scalable vector.

[Joe Ellis via Phabricator via cfe-commits]

joechrisellis updated this revision to Diff 308910.
joechrisellis added a comment.

Reduce debug info in `debug-declare-no-warnings-on-scalable-vectors.ll`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91806/new/

https://reviews.llvm.org/D91806

Files:
  llvm/lib/Transforms/Utils/Debugify.cpp
  llvm/lib/Transforms/Utils/Local.cpp
  
llvm/test/Transforms/InstCombine/debug-declare-no-warnings-on-scalable-vectors.ll

Index: llvm/test/Transforms/InstCombine/debug-declare-no-warnings-on-scalable-vectors.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/debug-declare-no-warnings-on-scalable-vectors.ll
@@ -0,0 +1,31 @@
+; RUN: opt -mtriple aarch64-gnu-linux -mattr=+sve -instcombine -S < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; If this check fails please read
+; clang/test/CodeGen/aarch64-sve-intrinsics/README for instructions on
+; how to resolve it.
+
+; WARN-NOT: warning
+
+; CHECK-LABEL: @debug_local_scalable(
+define  @debug_local_scalable( %tostore) {
+  %vx = alloca , align 16
+  call void @llvm.dbg.declare(metadata * %vx, metadata !3, metadata !DIExpression()), !dbg !5
+  store  %tostore, * %vx, align 16
+  %ret = call  @f(* %vx)
+  ret  %ret
+}
+
+declare  @f(*)
+
+; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
+!1 = !DIFile(filename: "/tmp/test.c", directory: "/tmp/")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !DILocalVariable(scope: !4)
+!4 = distinct !DISubprogram(unit: !0)
+!5 = !DILocation(scope: !4)
Index: llvm/lib/Transforms/Utils/Local.cpp
===
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -1368,16 +1368,22 @@
 /// least n bits.
 static bool valueCoversEntireFragment(Type *ValTy, DbgVariableIntrinsic *DII) {
   const DataLayout  = DII->getModule()->getDataLayout();
-  uint64_t ValueSize = DL.getTypeAllocSizeInBits(ValTy);
-  if (auto FragmentSize = DII->getFragmentSizeInBits())
-return ValueSize >= *FragmentSize;
+  TypeSize ValueSize = DL.getTypeAllocSizeInBits(ValTy);
+  if (Optional FragmentSize = DII->getFragmentSizeInBits()) {
+assert(!ValueSize.isScalable() &&
+   "Fragments don't work on scalable types.");
+return ValueSize.getFixedSize() >= *FragmentSize;
+  }
   // We can't always calculate the size of the DI variable (e.g. if it is a
   // VLA). Try to use the size of the alloca that the dbg intrinsic describes
   // intead.
   if (DII->isAddressOfVariable())
 if (auto *AI = dyn_cast_or_null(DII->getVariableLocation()))
-  if (auto FragmentSize = AI->getAllocationSizeInBits(DL))
-return ValueSize >= *FragmentSize;
+  if (Optional FragmentSize = AI->getAllocationSizeInBits(DL)) {
+assert(ValueSize.isScalable() == FragmentSize->isScalable() &&
+   "Both sizes should agree on the scalable flag.");
+return TypeSize::isKnownGE(ValueSize, *FragmentSize);
+  }
   // Could not determine size of variable. Conservatively return false.
   return false;
 }
Index: llvm/lib/Transforms/Utils/Debugify.cpp
===
--- llvm/lib/Transforms/Utils/Debugify.cpp
+++ llvm/lib/Transforms/Utils/Debugify.cpp
@@ -44,8 +44,9 @@
 
 raw_ostream () { return Quiet ? nulls() : errs(); }
 
-uint64_t getAllocSizeInBits(Module , Type *Ty) {
-  return Ty->isSized() ? M.getDataLayout().getTypeAllocSizeInBits(Ty) : 0;
+TypeSize getAllocSizeInBits(Module , Type *Ty) {
+  return Ty->isSized() ? M.getDataLayout().getTypeAllocSizeInBits(Ty)
+   : TypeSize::getFixed(0);
 }
 
 bool isFunctionSkipped(Function ) {
@@ -276,7 +277,7 @@
 return false;
 
   Type *Ty = V->getType();
-  uint64_t ValueOperandSize = getAllocSizeInBits(M, Ty);
+  TypeSize ValueOperandSize = getAllocSizeInBits(M, Ty);
   Optional DbgVarSize = DVI->getFragmentSizeInBits();
   if (!ValueOperandSize || !DbgVarSize)
 return false;
@@ -285,7 +286,7 @@
   if (Ty->isIntegerTy()) {
 auto Signedness = DVI->getVariable()->getSignedness();
 if (Signedness && *Signedness == DIBasicType::Signedness::Signed)
-  HasBadSize = ValueOperandSize < *DbgVarSize;
+  HasBadSize = ValueOperandSize.getFixedSize() < *DbgVarSize;
   } else {
 HasBadSize = ValueOperandSize != *DbgVarSize;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91806: [SVE] Remove warning from debug info on scalable vector.

[Joe Ellis via Phabricator via cfe-commits]

joechrisellis commandeered this revision.
joechrisellis edited reviewers, added: fpetrogalli; removed: joechrisellis.
joechrisellis added a comment.

I am finishing up this patch. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91806/new/

https://reviews.llvm.org/D91806

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91806: [SVE] Remove warning from debug info on scalable vector.

[Francesco Petrogalli via Phabricator via cfe-commits]

fpetrogalli updated this revision to Diff 308078.
fpetrogalli added a comment.

Reverted `getFragmentSizeInBits` to return an integral type and not `TypeSize`, 
because scalable variables cannot be part of structs/arrays - hence they cannot 
be pointed by fragments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91806/new/

https://reviews.llvm.org/D91806

Files:
  llvm/lib/Transforms/Utils/Debugify.cpp
  llvm/lib/Transforms/Utils/Local.cpp
  
llvm/test/Transforms/InstCombine/debug-declare-no-warnings-on-scalable-vectors.ll

Index: llvm/test/Transforms/InstCombine/debug-declare-no-warnings-on-scalable-vectors.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/debug-declare-no-warnings-on-scalable-vectors.ll
@@ -0,0 +1,42 @@
+; RUN: opt -mtriple aarch64-gnu-linux -mattr=+sve -instcombine -S < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; If this check fails please read
+; clang/test/CodeGen/aarch64-sve-intrinsics/README for instructions on
+; how to resolve it.
+
+; WARN-NOT: warning
+
+; CHECK-LABEL: @debug_local_scalable(
+define  @debug_local_scalable( %tostore) {
+  %vx = alloca , align 16
+  call void @llvm.dbg.declare(metadata * %vx, metadata !5, metadata !DIExpression()), !dbg !15
+  store  %tostore, * %vx, align 16
+  %ret = call  @f(* %vx)
+  ret  %ret
+}
+
+declare  @f(*)
+
+; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 12.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "/tmp/test.c", directory: "/tmp/")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !DILocalVariable(name: "vx", scope: !6, file: !7, line: 26, type: !8)
+!6 = distinct !DISubprogram(name: "debug_local_scalable", scope: null, file: !1, line: 25, scopeLine: 25, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!7 = !DIFile(filename: "test.c", directory: "/tmp/")
+!8 = !DIDerivedType(tag: DW_TAG_typedef, name: "svfloat64_t", file: !9, line: 56, baseType: !10)
+!9 = !DIFile(filename: "arm_sve.h", directory: "/tmp/")
+!10 = !DIDerivedType(tag: DW_TAG_typedef, name: "__SVFloat64_t", file: !1, baseType: !11)
+!11 = !DICompositeType(tag: DW_TAG_array_type, baseType: !12, flags: DIFlagVector, elements: !13)
+!12 = !DIBasicType(name: "double", size: 64, encoding: DW_ATE_float)
+!13 = !{!14}
+!14 = !DISubrange(lowerBound: 0, upperBound: !DIExpression(DW_OP_constu, 1, DW_OP_bregx, 46, 0, DW_OP_mul, DW_OP_constu, 1, DW_OP_minus))
+!15 = !DILocation(line: 26, column: 15, scope: !6)
Index: llvm/lib/Transforms/Utils/Local.cpp
===
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -1368,16 +1368,22 @@
 /// least n bits.
 static bool valueCoversEntireFragment(Type *ValTy, DbgVariableIntrinsic *DII) {
   const DataLayout  = DII->getModule()->getDataLayout();
-  uint64_t ValueSize = DL.getTypeAllocSizeInBits(ValTy);
-  if (auto FragmentSize = DII->getFragmentSizeInBits())
-return ValueSize >= *FragmentSize;
+  TypeSize ValueSize = DL.getTypeAllocSizeInBits(ValTy);
+  if (Optional FragmentSize = DII->getFragmentSizeInBits()) {
+assert(!ValueSize.isScalable() &&
+   "Fragments don't work on scalable types.");
+return ValueSize.getFixedSize() >= *FragmentSize;
+  }
   // We can't always calculate the size of the DI variable (e.g. if it is a
   // VLA). Try to use the size of the alloca that the dbg intrinsic describes
   // intead.
   if (DII->isAddressOfVariable())
 if (auto *AI = dyn_cast_or_null(DII->getVariableLocation()))
-  if (auto FragmentSize = AI->getAllocationSizeInBits(DL))
-return ValueSize >= *FragmentSize;
+  if (Optional FragmentSize = AI->getAllocationSizeInBits(DL)) {
+assert(ValueSize.isScalable() == FragmentSize->isScalable() &&
+   "Both sizes should agree on the scalable flag.");
+return TypeSize::isKnownGE(ValueSize, *FragmentSize);
+  }
   // Could not determine size of variable. Conservatively return false.
   return false;
 }
Index: llvm/lib/Transforms/Utils/Debugify.cpp
===
--- llvm/lib/Transforms/Utils/Debugify.cpp
+++ llvm/lib/Transforms/Utils/Debugify.cpp
@@ -44,8 +44,9 @@
 
 raw_ostream () { return Quiet ? nulls() : errs(); }
 
-uint64_t getAllocSizeInBits(Module , Type *Ty) {
-  return Ty->isSized() ? M.getDataLayout().getTypeAllocSizeInBits(Ty) : 0;
+TypeSize getAllocSizeInBits(Module , Type 

[PATCH] D91806: [SVE] Remove warning from debug info on scalable vector.

[Francesco Petrogalli via Phabricator via cfe-commits]

fpetrogalli added a comment.

Hi @sdesmalen

I have extracted D92020  to implement only the 
change of interface for `AllocaInst::getAllocationSizeInBits`.

I wanted to extract also the interface change in 
`DbgVariableIntrinsic::getFragmentSizeInBits()` but such change would require 
also to change `valueCoversEntireFragment`: essentially, this is already what I 
am doing in this patch.,

> This patch needs to be retitled to what this is actually doing: changing the 
> getTypeAllocationSizeInBits and getFragmentSizeInBits to return a TypeSize 
> instead of unsigned.

Given that the only change resulting from this patch is removing the warning, I 
think it makes sense to keep the summary of the patch as it is.

I have added a comment in `DbgVariableIntrinsic::getFragmentSizeInBits()` 
explaining that to get full support for scalable types in 
`DbgVariableIntrinsic::getFragmentSizeInBits()` we should update the 
`DIVariable::getSizeInBits` to carry the scalable flag for scalable variables. 
I am happy to continue in that direction (it seems an extensive change with 
quite a few implications), I just wanted to double check whether 1. you agree 
on the need to change `DIVariable::getSizeInBits` to privide `TypeSize` info, 
and 2.  if you think if you are happy for the warning fix (this patch)  to go 
in before the `DIVariable::getSizeInBits` interface change, or 3. do you see a 
third way? :)

Cheers,

Francesco


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91806/new/

https://reviews.llvm.org/D91806

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91806: [SVE] Remove warning from debug info on scalable vector.

[Francesco Petrogalli via Phabricator via cfe-commits]

fpetrogalli updated this revision to Diff 307329.
fpetrogalli added a comment.

Add comment to `DbgVariableIntrinsic::getFragmentSizeInBits()`. NFC


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91806/new/

https://reviews.llvm.org/D91806

Files:
  llvm/include/llvm/IR/IntrinsicInst.h
  llvm/lib/IR/IntrinsicInst.cpp
  llvm/lib/Transforms/Utils/Debugify.cpp
  llvm/lib/Transforms/Utils/Local.cpp
  
llvm/test/Transforms/InstCombine/debug-declare-no-warnings-on-scalable-vectors.ll

Index: llvm/test/Transforms/InstCombine/debug-declare-no-warnings-on-scalable-vectors.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/debug-declare-no-warnings-on-scalable-vectors.ll
@@ -0,0 +1,42 @@
+; RUN: opt -mtriple aarch64-gnu-linux -mattr=+sve -instcombine -S < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; If this check fails please read
+; clang/test/CodeGen/aarch64-sve-intrinsics/README for instructions on
+; how to resolve it.
+
+; WARN-NOT: warning
+
+; CHECK-LABEL: @debug_local_scalable(
+define  @debug_local_scalable( %tostore) {
+  %vx = alloca , align 16
+  call void @llvm.dbg.declare(metadata * %vx, metadata !5, metadata !DIExpression()), !dbg !15
+  store  %tostore, * %vx, align 16
+  %ret = call  @f(* %vx)
+  ret  %ret
+}
+
+declare  @f(*)
+
+; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 12.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "/tmp/test.c", directory: "/tmp/")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !DILocalVariable(name: "vx", scope: !6, file: !7, line: 26, type: !8)
+!6 = distinct !DISubprogram(name: "debug_local_scalable", scope: null, file: !1, line: 25, scopeLine: 25, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!7 = !DIFile(filename: "test.c", directory: "/tmp/")
+!8 = !DIDerivedType(tag: DW_TAG_typedef, name: "svfloat64_t", file: !9, line: 56, baseType: !10)
+!9 = !DIFile(filename: "arm_sve.h", directory: "/tmp/")
+!10 = !DIDerivedType(tag: DW_TAG_typedef, name: "__SVFloat64_t", file: !1, baseType: !11)
+!11 = !DICompositeType(tag: DW_TAG_array_type, baseType: !12, flags: DIFlagVector, elements: !13)
+!12 = !DIBasicType(name: "double", size: 64, encoding: DW_ATE_float)
+!13 = !{!14}
+!14 = !DISubrange(lowerBound: 0, upperBound: !DIExpression(DW_OP_constu, 1, DW_OP_bregx, 46, 0, DW_OP_mul, DW_OP_constu, 1, DW_OP_minus))
+!15 = !DILocation(line: 26, column: 15, scope: !6)
Index: llvm/lib/Transforms/Utils/Local.cpp
===
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -1368,16 +1368,22 @@
 /// least n bits.
 static bool valueCoversEntireFragment(Type *ValTy, DbgVariableIntrinsic *DII) {
   const DataLayout  = DII->getModule()->getDataLayout();
-  uint64_t ValueSize = DL.getTypeAllocSizeInBits(ValTy);
-  if (auto FragmentSize = DII->getFragmentSizeInBits())
-return ValueSize >= *FragmentSize;
+  TypeSize ValueSize = DL.getTypeAllocSizeInBits(ValTy);
+  if (Optional FragmentSize = DII->getFragmentSizeInBits()) {
+assert(ValueSize.isScalable() == FragmentSize->isScalable() &&
+   "Both sizes should agree on the scalable flag.");
+return TypeSize::isKnownGE(ValueSize, *FragmentSize);
+  }
   // We can't always calculate the size of the DI variable (e.g. if it is a
   // VLA). Try to use the size of the alloca that the dbg intrinsic describes
   // intead.
   if (DII->isAddressOfVariable())
 if (auto *AI = dyn_cast_or_null(DII->getVariableLocation()))
-  if (auto FragmentSize = AI->getAllocationSizeInBits(DL))
-return ValueSize >= *FragmentSize;
+  if (Optional FragmentSize = AI->getAllocationSizeInBits(DL)) {
+assert(ValueSize.isScalable() == FragmentSize->isScalable() &&
+   "Both sizes should agree on the scalable flag.");
+return TypeSize::isKnownGE(ValueSize, *FragmentSize);
+  }
   // Could not determine size of variable. Conservatively return false.
   return false;
 }
Index: llvm/lib/Transforms/Utils/Debugify.cpp
===
--- llvm/lib/Transforms/Utils/Debugify.cpp
+++ llvm/lib/Transforms/Utils/Debugify.cpp
@@ -44,8 +44,9 @@
 
 raw_ostream () { return Quiet ? nulls() : errs(); }
 
-uint64_t getAllocSizeInBits(Module , Type *Ty) {
-  return Ty->isSized() ? M.getDataLayout().getTypeAllocSizeInBits(Ty) : 0;
+TypeSize getAllocSizeInBits(Module , Type *Ty) {
+ 

[PATCH] D91806: [SVE] Remove warning from debug info on scalable vector.

[Francesco Petrogalli via Phabricator via cfe-commits]

fpetrogalli updated this revision to Diff 307306.
fpetrogalli added a comment.

Rebase on top of D92020 . NFC.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91806/new/

https://reviews.llvm.org/D91806

Files:
  llvm/include/llvm/IR/IntrinsicInst.h
  llvm/lib/IR/IntrinsicInst.cpp
  llvm/lib/Transforms/Utils/Debugify.cpp
  llvm/lib/Transforms/Utils/Local.cpp
  
llvm/test/Transforms/InstCombine/debug-declare-no-warnings-on-scalable-vectors.ll

Index: llvm/test/Transforms/InstCombine/debug-declare-no-warnings-on-scalable-vectors.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/debug-declare-no-warnings-on-scalable-vectors.ll
@@ -0,0 +1,42 @@
+; RUN: opt -mtriple aarch64-gnu-linux -mattr=+sve -instcombine -S < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; If this check fails please read
+; clang/test/CodeGen/aarch64-sve-intrinsics/README for instructions on
+; how to resolve it.
+
+; WARN-NOT: warning
+
+; CHECK-LABEL: @debug_local_scalable(
+define  @debug_local_scalable( %tostore) {
+  %vx = alloca , align 16
+  call void @llvm.dbg.declare(metadata * %vx, metadata !5, metadata !DIExpression()), !dbg !15
+  store  %tostore, * %vx, align 16
+  %ret = call  @f(* %vx)
+  ret  %ret
+}
+
+declare  @f(*)
+
+; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 12.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "/tmp/test.c", directory: "/tmp/")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !DILocalVariable(name: "vx", scope: !6, file: !7, line: 26, type: !8)
+!6 = distinct !DISubprogram(name: "debug_local_scalable", scope: null, file: !1, line: 25, scopeLine: 25, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!7 = !DIFile(filename: "test.c", directory: "/tmp/")
+!8 = !DIDerivedType(tag: DW_TAG_typedef, name: "svfloat64_t", file: !9, line: 56, baseType: !10)
+!9 = !DIFile(filename: "arm_sve.h", directory: "/tmp/")
+!10 = !DIDerivedType(tag: DW_TAG_typedef, name: "__SVFloat64_t", file: !1, baseType: !11)
+!11 = !DICompositeType(tag: DW_TAG_array_type, baseType: !12, flags: DIFlagVector, elements: !13)
+!12 = !DIBasicType(name: "double", size: 64, encoding: DW_ATE_float)
+!13 = !{!14}
+!14 = !DISubrange(lowerBound: 0, upperBound: !DIExpression(DW_OP_constu, 1, DW_OP_bregx, 46, 0, DW_OP_mul, DW_OP_constu, 1, DW_OP_minus))
+!15 = !DILocation(line: 26, column: 15, scope: !6)
Index: llvm/lib/Transforms/Utils/Local.cpp
===
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -1368,16 +1368,22 @@
 /// least n bits.
 static bool valueCoversEntireFragment(Type *ValTy, DbgVariableIntrinsic *DII) {
   const DataLayout  = DII->getModule()->getDataLayout();
-  uint64_t ValueSize = DL.getTypeAllocSizeInBits(ValTy);
-  if (auto FragmentSize = DII->getFragmentSizeInBits())
-return ValueSize >= *FragmentSize;
+  TypeSize ValueSize = DL.getTypeAllocSizeInBits(ValTy);
+  if (Optional FragmentSize = DII->getFragmentSizeInBits()) {
+assert(ValueSize.isScalable() == FragmentSize->isScalable() &&
+   "Both sizes should agree on the scalable flag.");
+return TypeSize::isKnownGE(ValueSize, *FragmentSize);
+  }
   // We can't always calculate the size of the DI variable (e.g. if it is a
   // VLA). Try to use the size of the alloca that the dbg intrinsic describes
   // intead.
   if (DII->isAddressOfVariable())
 if (auto *AI = dyn_cast_or_null(DII->getVariableLocation()))
-  if (auto FragmentSize = AI->getAllocationSizeInBits(DL))
-return ValueSize >= *FragmentSize;
+  if (Optional FragmentSize = AI->getAllocationSizeInBits(DL)) {
+assert(ValueSize.isScalable() == FragmentSize->isScalable() &&
+   "Both sizes should agree on the scalable flag.");
+return TypeSize::isKnownGE(ValueSize, *FragmentSize);
+  }
   // Could not determine size of variable. Conservatively return false.
   return false;
 }
Index: llvm/lib/Transforms/Utils/Debugify.cpp
===
--- llvm/lib/Transforms/Utils/Debugify.cpp
+++ llvm/lib/Transforms/Utils/Debugify.cpp
@@ -44,8 +44,9 @@
 
 raw_ostream () { return Quiet ? nulls() : errs(); }
 
-uint64_t getAllocSizeInBits(Module , Type *Ty) {
-  return Ty->isSized() ? M.getDataLayout().getTypeAllocSizeInBits(Ty) : 0;
+TypeSize getAllocSizeInBits(Module , Type *Ty) {
+  

[PATCH] D91806: [SVE] Remove warning from debug info on scalable vector.

[Francesco Petrogalli via Phabricator via cfe-commits]

fpetrogalli marked an inline comment as done.
fpetrogalli added inline comments.



Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:589
   sort(FrameData.Allocas, [&](const auto , const auto ) {
-return GetAllocaSize(Iter1) > GetAllocaSize(Iter2);
+return TypeSize::isKnownGT(GetAllocaSize(Iter1), GetAllocaSize(Iter2));
   });

We don't need to check whether the scalable flag matches here because we are 
sorting a container that (potentially) have both types of vectors.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91806/new/

https://reviews.llvm.org/D91806

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91806: [SVE] Remove warning from debug info on scalable vector.

[Francesco Petrogalli via Phabricator via cfe-commits]

fpetrogalli updated this revision to Diff 306715.
fpetrogalli edited the summary of this revision.
fpetrogalli added a comment.

I have added assertions around before `TypeSize` comparisons where it made 
sense to do so, but not in the lambda used in the `sort` invocation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91806/new/

https://reviews.llvm.org/D91806

Files:
  llvm/include/llvm/IR/Instructions.h
  llvm/include/llvm/IR/IntrinsicInst.h
  llvm/lib/IR/Instructions.cpp
  llvm/lib/IR/IntrinsicInst.cpp
  llvm/lib/Transforms/Coroutines/CoroFrame.cpp
  llvm/lib/Transforms/Utils/Debugify.cpp
  llvm/lib/Transforms/Utils/Local.cpp
  
llvm/test/Transforms/InstCombine/debug-declare-no-warnings-on-scalable-vectors.ll

Index: llvm/test/Transforms/InstCombine/debug-declare-no-warnings-on-scalable-vectors.ll
===
--- /dev/null
+++ llvm/test/Transforms/InstCombine/debug-declare-no-warnings-on-scalable-vectors.ll
@@ -0,0 +1,42 @@
+; RUN: opt -mtriple aarch64-gnu-linux -mattr=+sve -instcombine -S < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; If this check fails please read
+; clang/test/CodeGen/aarch64-sve-intrinsics/README for instructions on
+; how to resolve it.
+
+; WARN-NOT: warning
+
+; CHECK-LABEL: @debug_local_scalable(
+define  @debug_local_scalable( %tostore) {
+  %vx = alloca , align 16
+  call void @llvm.dbg.declare(metadata * %vx, metadata !5, metadata !DIExpression()), !dbg !15
+  store  %tostore, * %vx, align 16
+  %ret = call  @f(* %vx)
+  ret  %ret
+}
+
+declare  @f(*)
+
+; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 12.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "/tmp/test.c", directory: "/tmp/")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !DILocalVariable(name: "vx", scope: !6, file: !7, line: 26, type: !8)
+!6 = distinct !DISubprogram(name: "debug_local_scalable", scope: null, file: !1, line: 25, scopeLine: 25, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!7 = !DIFile(filename: "test.c", directory: "/tmp/")
+!8 = !DIDerivedType(tag: DW_TAG_typedef, name: "svfloat64_t", file: !9, line: 56, baseType: !10)
+!9 = !DIFile(filename: "arm_sve.h", directory: "/tmp/")
+!10 = !DIDerivedType(tag: DW_TAG_typedef, name: "__SVFloat64_t", file: !1, baseType: !11)
+!11 = !DICompositeType(tag: DW_TAG_array_type, baseType: !12, flags: DIFlagVector, elements: !13)
+!12 = !DIBasicType(name: "double", size: 64, encoding: DW_ATE_float)
+!13 = !{!14}
+!14 = !DISubrange(lowerBound: 0, upperBound: !DIExpression(DW_OP_constu, 1, DW_OP_bregx, 46, 0, DW_OP_mul, DW_OP_constu, 1, DW_OP_minus))
+!15 = !DILocation(line: 26, column: 15, scope: !6)
Index: llvm/lib/Transforms/Utils/Local.cpp
===
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -1368,16 +1368,22 @@
 /// least n bits.
 static bool valueCoversEntireFragment(Type *ValTy, DbgVariableIntrinsic *DII) {
   const DataLayout  = DII->getModule()->getDataLayout();
-  uint64_t ValueSize = DL.getTypeAllocSizeInBits(ValTy);
-  if (auto FragmentSize = DII->getFragmentSizeInBits())
-return ValueSize >= *FragmentSize;
+  TypeSize ValueSize = DL.getTypeAllocSizeInBits(ValTy);
+  if (Optional FragmentSize = DII->getFragmentSizeInBits()) {
+assert(ValueSize.isScalable() == FragmentSize->isScalable() &&
+   "Both sizes should agree on the scalable flag.");
+return TypeSize::isKnownGE(ValueSize, *FragmentSize);
+  }
   // We can't always calculate the size of the DI variable (e.g. if it is a
   // VLA). Try to use the size of the alloca that the dbg intrinsic describes
   // intead.
   if (DII->isAddressOfVariable())
 if (auto *AI = dyn_cast_or_null(DII->getVariableLocation()))
-  if (auto FragmentSize = AI->getAllocationSizeInBits(DL))
-return ValueSize >= *FragmentSize;
+  if (Optional FragmentSize = AI->getAllocationSizeInBits(DL)) {
+assert(ValueSize.isScalable() == FragmentSize->isScalable() &&
+   "Both sizes should agree on the scalable flag.");
+return TypeSize::isKnownGE(ValueSize, *FragmentSize);
+  }
   // Could not determine size of variable. Conservatively return false.
   return false;
 }
Index: llvm/lib/Transforms/Utils/Debugify.cpp
===
--- llvm/lib/Transforms/Utils/Debugify.cpp
+++ llvm/lib/Transforms/Utils/Debugify.cpp
@@ -44,8 

[PATCH] D91806: [SVE] Remove warning from debug info on scalable vector.

[Francesco Petrogalli via Phabricator via cfe-commits]

fpetrogalli marked an inline comment as done.
fpetrogalli added a comment.

Thank you for the review @sdesmalen

1. I have replied to the comment about `auto` with my reason for removing it.
2. I removed the C test, as the LL file is enough.
3. The third comment requires more investigation, I'll get back to you to see 
the use of the `TypeSize` comparisons.

Thank you!




Comment at: llvm/lib/IR/IntrinsicInst.cpp:56
+Optional DbgVariableIntrinsic::getFragmentSizeInBits() const {
+  if (Optional Fragment =
+  getExpression()->getFragmentInfo())

sdesmalen wrote:
> Change back to `auto` ?
I'd like to keep this, because it seems to me the use of auto in this case 
didn't fit in the cases mentioned in 
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable.
 There is no direct type deduction from the context around the initialization 
of the variable `Fragment`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91806/new/

https://reviews.llvm.org/D91806

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91806: [SVE] Remove warning from debug info on scalable vector.

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added a comment.

This patch needs to be retitled to what this is actually doing: changing the 
getTypeAllocationSizeInBits and getFragmentSizeInBits to return a TypeSize 
instead of `unsigned`.
It would be even better if you can split those up into two patches with 
separate tests for each one of them.




Comment at: clang/test/CodeGen/aarch64-sve-acle-rel-note.c:1
+// REQUIRES: aarch64-registered-target
+// RUN: %clang --target=aarch64-linux-gnu -march=armv8-a+sve -S -emit-llvm -o 
- -c %s -Werror -Wall -g -O0 2>%t | FileCheck %s

I'm not entirely sure if it is acceptable to have a clang test that generates 
assembly, but in any case I think this test needs to be added in a separate 
patch.



Comment at: llvm/lib/IR/IntrinsicInst.cpp:56
+Optional DbgVariableIntrinsic::getFragmentSizeInBits() const {
+  if (Optional Fragment =
+  getExpression()->getFragmentInfo())

Change back to `auto` ?



Comment at: llvm/lib/Transforms/Utils/Local.cpp:1373
+  if (Optional FragmentSize = DII->getFragmentSizeInBits())
+return TypeSize::isKnownGE(ValueSize, *FragmentSize);
   // We can't always calculate the size of the DI variable (e.g. if it is a

Should the scalable flag match? Same question for all the other cases in this 
patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91806/new/

https://reviews.llvm.org/D91806

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits