Page MenuHomePhabricator

[Transforms] Fix a wrong debug info intrinsic call in `mem2reg` pass for ref 128-bit
AbandonedPublic

Authored by slydiman on Apr 29 2022, 12:27 PM.

Details

Summary

Calling device function with reference to __int128_t parameters emits incorrect debug info intrinsic call.
This happens in the mem2reg pass, while eliminating allocas it also tries to update the debug intrinsics.
For a reference parameter, the fragment size is the size of the underlying object. When eliminating alloca for double pointer to the object with a pointer to another object, the code in question wrongly assumes that a pointer doesn't fully cover the fragment.

This patch fixes that and adds a new test to cover this case.

Diff Detail

Unit TestsFailed

TimeTest
60,370 msx64 debian > Clang.Driver::fsanitize.c
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -target x86_64-linux-gnu -fsanitize=undefined -fsanitize-trap=undefined /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/fsanitize.c -### 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/clang/test/Driver/fsanitize.c --check-prefix=CHECK-UNDEFINED-TRAP
60,240 msx64 debian > Clang.OpenMP::target_teams_distribute_parallel_for_simd_codegen_registration.cpp
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/15.0.0/include -nostdsysteminc -no-opaque-pointers -verify -fopenmp -fopenmp-version=45 -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-ibm-linux-gnu -emit-llvm /var/lib/buildkite-agent/builds/llvm-project/clang/test/OpenMP/target_teams_distribute_parallel_for_simd_codegen_registration.cpp -o - | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --allow-unused-prefixes /var/lib/buildkite-agent/builds/llvm-project/clang/test/OpenMP/target_teams_distribute_parallel_for_simd_codegen_registration.cpp
50 msx64 debian > Polly.ScopDetect::dot-scops-npm.ll
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/opt -polly-process-unprofitable -polly-remarks-minimal -polly-use-llvm-names -polly-import-jscop-dir=/var/lib/buildkite-agent/builds/llvm-project/polly/test/ScopDetect -polly-codegen-verify "-passes=polly-scop-printer" -disable-output < /var/lib/buildkite-agent/builds/llvm-project/polly/test/ScopDetect/dot-scops-npm.ll

Event Timeline

slydiman created this revision.Apr 29 2022, 12:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2022, 12:27 PM
slydiman requested review of this revision.Apr 29 2022, 12:27 PM
slydiman added a reviewer: tra.May 5 2022, 6:57 PM
tra added a subscriber: dblaikie.May 6 2022, 12:19 PM

I'm not familiar with debug info at all.
@dblaikie -- does this make sense to you? If it matters, i128 in NVPTX is not a native register type and is passed around as a pair of i64.

jmorse added a subscriber: jmorse.May 13 2022, 7:53 AM

While the patch does fix the immediate issue, I'm not certain that disabling the valueCoversEntireFragment check in the context of pointers is a general solution. Otherwise generic code that does something strange (like breaking up allocas) would expect dbg.value's to become undef, and not doing that for certain types risks causing unexpected behaviour in the future. It's also good practice to be conservative in what input we accept here.

It seems that the "size" field of DIDerivedType nodes is optional -- I'm not sure why. Regardless, by adding "size: 64" to !10 in your test, an unpatched opt produces the correct output. Are you working from a source-language reproducer, and wouldn't the more important bug be whatever caused the DIDerivedType size field to either be unassigned, or dropped?

(It's also not clear to me why LLVM has chosen to not have a DIDerivedType specific getSizeInBits field, as it would be able to identify the type size from it's tag being a pointer or reference. However, it looks like the existing design expects the size to be provided in the "size" field, so we should try fixing that first).

slydiman updated this revision to Diff 429577.May 15 2022, 4:23 PM

I have simplified the test and made it generic.

I tried to update the size of DIDerivedType in llvm/lib/AsmParser/LLParser.cpp:

bool LLParser::parseDIDerivedType(MDNode *&Result, bool IsDistinct) {
  ...

  if (tag.Val == dwarf::DW_TAG_reference_type && size.Val == 0)
    size = M->getDataLayout().getPointerSizeInBits();

  Result = GET_OR_DISTINCT(DIDerivedType,
                           (Context, tag.Val, name.Val, file.Val, line.Val,
                            scope.Val, baseType.Val, size.Val, align.Val,
                            offset.Val, DWARFAddressSpace, flags.Val,
                            extraData.Val, annotations.Val));
  return false;
}

It solved the problem too, but it caused the appearance of this field size for DIDerivedType in the output:
!10 = !DIDerivedType(tag: DW_TAG_reference_type, baseType: !4) -> !10 = !DIDerivedType(tag: DW_TAG_reference_type, baseType: !4, size: 64)

ormris removed a subscriber: ormris.May 16 2022, 10:49 AM

Hi @slydiman,

It looks like the clang front end emits a size field for reference types. e.g.

$ clang++ --version
clang version 15.0.0 (https://github.com/llvm/llvm-project.git 8e9528cb544a3d70cea96f1f99318643095a1410)
Target: x86_64-unknown-linux-gnu

$ cat test.cpp
void fun(__int128_t &i) {}

$ clang++ test.cpp -Xclang -disable-llvm-passes -g -emit-llvm -S -o -
...
!11 = !DIDerivedType(tag: DW_TAG_reference_type, baseType: !12, size: 64)
!12 = !DIDerivedType(tag: DW_TAG_typedef, name: "__int128_t", file: !13, baseType: !14)
!13 = !DIFile(filename: "test.cpp", directory: "/home/och/dev/bugs/scratch")
!14 = !DIBasicType(name: "__int128", size: 128, encoding: DW_ATE_signed)
!15 = !{}
!16 = !DILocalVariable(name: "i", arg: 1, scope: !8, file: !1, line: 1, type: !11)

Notice that i's (!16) type is !11 which is a DW_TAG_reference_type type DIDerivedType with a field size: 64. IIUC this means valueCoversEntireFragment should work correctly for this type without your patch.

Perhaps this can be fixed by patching your front end. Was your reproducer created using the clang front end?

slydiman abandoned this revision.Thu, Jun 9, 11:53 AM