Page MenuHomePhabricator

[MemIntrinsics] Add an API to get elementtype attribute value. NFC.
AbandonedPublic

Authored by dantrushin on May 16 2022, 7:48 AM.

Details

Summary

Add simple wrappers to make CallBase::getParamElementType() available
to users of Memory Intrinsics. Some environments may need it when
opaque pointers are enabled.
Right now there are no users of these interfaces in tree, so this is NFC.

Diff Detail

Unit TestsFailed

TimeTest
60,400 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

Event Timeline

dantrushin created this revision.May 16 2022, 7:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2022, 7:48 AM
dantrushin requested review of this revision.May 16 2022, 7:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2022, 7:48 AM
nikic requested changes to this revision.May 16 2022, 12:02 PM

We generally do not add in-tree methods if they have no in-tree users. I'm also rather skeptical about what you're trying to do here -- this would need some more detailed explanation of the use case.

This revision now requires changes to proceed.May 16 2022, 12:02 PM
dantrushin added a comment.EditedMay 16 2022, 12:19 PM

We generally do not add in-tree methods if they have no in-tree users. I'm also rather skeptical about what you're trying to do here -- this would need some more detailed explanation of the use case.

Intrinsics allow to specify element type of its pointer arguments via elementtype attribute.
Atomic memory intrinsics hide this possibility behind quite complicated hierarchy. This patch makes it accessible for any interested party. I don't see what's wrong with that?

We generally do not add in-tree methods if they have no in-tree users. I'm also rather skeptical about what you're trying to do here -- this would need some more detailed explanation of the use case.

Intrinsics allow to specify element type of its pointer arguments via elementtype attribute.
Atomic memory intrinsics hide this possibility behind quite complicated hierarchy. This patch makes it accessible for any interested party. I don't see what's wrong with that?

Use of elementtype is discouraged -- it should only be used if there is no other solution. I don't want to add APIs legitimizing its use in this context without justification. For example, consider the llvm.memcpy.element.unordered.atomic intrinsics which specify an element size as an additional argument, rather than a precise element type.

Use of elementtype is discouraged -- it should only be used if there is no other solution. I don't want to add APIs legitimizing its use in this context without justification. For example, consider the llvm.memcpy.element.unordered.atomic intrinsics which specify an element size as an additional argument, rather than a precise element type.

Copying arrays of pointers in environments with moving garbage collector is different from copying of 'primitive' types of the same types.
Well, I understand that upstream may not take it as a 'justification'.

Anyway, I think that discouraged part is worth more explicit mentioning in opaque pointers guide - thhat was not clear to me while reading it.

dantrushin abandoned this revision.May 19 2022, 6:25 AM

Looks like this has to live downstream. Still I think if upstream provides some public api (CallBase::getParamElementType) it should be available to all users

apilipenko added a subscriber: apilipenko.EditedMay 25 2022, 12:30 PM

Copying arrays of pointers in environments with moving garbage collector is different from copying of 'primitive' types of the same types.
Well, I understand that upstream may not take it as a 'justification'.

GC support is a supported feature of LLVM upstream (https://llvm.org/docs/GarbageCollection.html). I think this is a totally fair justification for elementtype on memory transfer intrinsics. I would argue that elementtype might be a requirement for some of the GC strategies because without the type information we can't choose the right lowering for the intrinsic.

does using the size parameter not work for your use case? do you specifically need the type?

anna added a comment.May 26 2022, 9:39 AM

does using the size parameter not work for your use case? do you specifically need the type?

Unfortunately, size is not a precise way of differentiating primitives from array of pointers. We can still have the size of a primitive being the same as the size of an array (consider array with 1 element).
The specific use case for us is that when we lower these intrinsics, we need to know if we need to add the GC support for this (done in IR itself) or not. With opaque pointers without elementttype in the intrinsic, we lose this knowledge,