[InstCombine] Update valueCoversEntireFragment to use TypeSize

[peterwaller-arm]

Summary

• Update valueCoversEntireFragment to use TypeSize.
• Add a regression test.
• Assertions have been added to protect untested codepaths.

[fpetrogalli]

I updated the RUN lines of the .ll test to prevent the failure at https://reviews.llvm.org/harbormaster/unit/view/196899/.

[fpetrogalli]

FYI: I am trying to figure out this failure that does not appear on my machine: https://reviews.llvm.org/harbormaster/unit/view/196974/

[fpetrogalli]

This last change is trying to remove the failure at https://reviews.llvm.org/harbormaster/unit/view/196974/
I haven’t been able to reproduce such failure on my dev machine, so I just changed the RUN lines in the test (added -c, used 2>%t) to see if the bot is going to be happy with the new invocation.

[fpetrogalli]

I have removed the C test, as the LL test is enough to test the changes I have done in Local.cpp.

I can confirm that with this patch, the release note ton'r raise any warnings when compiling with debug info:

frapet01@man-08:~/projects/upstream-clang/build-clang$ cat ../release-notes.c
#include <arm_sve.h>

void VLA_add_arrays(double *x, double *y, double *out, unsigned N) {
  for (unsigned i = 0; i < N; i += svcntd()) {
    svbool_t Pg = svwhilelt_b64(i, N);
    svfloat64_t vx = svld1(Pg, &x[i]);
    svfloat64_t vy = svld1(Pg, &y[i]);
    svfloat64_t vout = svadd_x(Pg, vx, vy);
    svst1(Pg, &out[i], vout);
  }
}
frapet01@man-08:~/projects/upstream-clang/build-clang$ ./bin/clang --target=aarch64-gnu-linux -march=armv8-a+sve -c -g -S -O3 -o /dev/null ../release-notes.c
frapet01@man-08:~/projects/upstream-clang/build-clang$

When the patch will end up into a nightly build of compiler explorer, the compilation at this link should not raise any warnings: https://godbolt.org/z/zebzKM

[sdesmalen]

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.

clang/test/CodeGen/aarch64-sve-acle-rel-note.c

1 ↗

(On Diff #306643)

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.

llvm/lib/IR/IntrinsicInst.cpp

56 ↗

(On Diff #306643)

Change back to auto ?

llvm/lib/Transforms/Utils/Local.cpp

1345

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

[fpetrogalli]

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!

llvm/lib/IR/IntrinsicInst.cpp

56 ↗

(On Diff #306643)

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.

[fpetrogalli]

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.

[fpetrogalli]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

589 ↗

(On Diff #306684)

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.

[fpetrogalli]

Rebase on top of D92020. NFC.

[fpetrogalli]

Add comment to DbgVariableIntrinsic::getFragmentSizeInBits(). NFC

[fpetrogalli]

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

[fpetrogalli]

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.

[joechrisellis]

I am finishing up this patch. 😄

[joechrisellis]

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

[sdesmalen]

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.

[joechrisellis]

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.

[peterwaller-arm]

I've taken over the differential.

Simplify things a bit more, remove an untested remnant created from before the patch split.

[peterwaller-arm]

Attempting to update commit message with arc diff --verbatim.

[sdesmalen]

llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll

14

After your changes, what line causes this warning to be emitted? Does this test also pass with asserts enabled?

[peterwaller-arm]

llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll

14

It's a -NOT check, so it's not being emitted. Yes, the test passes even with asserts enabled.

I assume the question is motivated by putting the check next to the line which causes the warning(?).

[peterwaller-arm]

Update for review comment.

• Generalize WARN-NOT to disallow any warnings, not just those with specific text.

[peterwaller-arm]

llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll

14

Discussed out of band. Sander's interest was to disallow any warning output, as with other SVE ASM-NOT tests in the clang directory.

[aprantl]

this lgtm with all other reviewer's concerns addressed.

[sdesmalen]

LGTM

llvm/lib/Transforms/Utils/Local.cpp

 /// This is primarily intended as a helper for the different
 /// ConvertDebugDeclareToDebugValue functions. The dbg.declare/dbg.addr that is
 /// converted describes an alloca'd variable, so we need to use the
 /// alloc size of the value when doing the comparison. E.g. an i1 value will be
 /// identified as covering an n-bit fragment, if the store size of i1 is at
 /// least n bits.
 static bool valueCoversEntireFragment(Type *ValTy, DbgVariableIntrinsic *DII) {
   const DataLayout &DL = DII->getModule()->getDataLayout();
-  uint64_t ValueSize = DL.getTypeAllocSizeInBits(ValTy);
-  if (auto FragmentSize = DII->getFragmentSizeInBits())
-    return ValueSize >= *FragmentSize;
+  TypeSize ValueSize = DL.getTypeAllocSizeInBits(ValTy);
+  if (Optional<uint64_t> 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<AllocaInst>(DII->getVariableLocation()))
-      if (auto FragmentSize = AI->getAllocationSizeInBits(DL))
-        return ValueSize >= *FragmentSize;
+      if (Optional<TypeSize> 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;
 }
 
 /// Produce a DebugLoc to use for each dbg.declare/inst pair that are promoted
 /// to a dbg.value. Because no machine insts can come from debug intrinsics,
 /// only the scope and inlinedAt is significant. Zero line numbers are used in
 /// case this DebugLoc leaks into any adjacent instructions.

llvm/test/Transforms/InstCombine/debuginfo-scalable-typesize.ll

+; RUN: opt -instcombine -S < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; This test is defending against a TypeSize message raised in the method
+; `valueCoversEntireFragment` in Local.cpp because of an implicit cast from
+; `TypeSize` to `uint64_t`. This particular TypeSize message 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.
+; This test must not produce any warnings. Prior to this test being introduced,
+; it produced a warning containing the text "TypeSize is not scalable".
+; WARN-NOT: warning:
+
+; CHECK-LABEL: @debug_local_scalable(
+define <vscale x 2 x double> @debug_local_scalable(<vscale x 2 x double> %tostore) {
+  %vx = alloca <vscale x 2 x double>, align 16
+  call void @llvm.dbg.declare(metadata <vscale x 2 x double>* %vx, metadata !3, metadata !DIExpression()), !dbg !5
+  store <vscale x 2 x double> %tostore, <vscale x 2 x double>* %vx, align 16
+  %ret = call <vscale x 2 x double> @f(<vscale x 2 x double>* %vx)
+  ret <vscale x 2 x double> %ret
+}
+
+declare <vscale x 2 x double> @f(<vscale x 2 x double>*)
+
+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)