This is an archive of the discontinued LLVM Phabricator instance.

Make sure the right parameter extension attributes are added in various instrumentation passes.
ClosedPublic

Authored by jonpa on Sep 15 2022, 8:48 AM.

Details

Summary

Initial patch (in progress):

  • It seemed like a good idea to make setArgExtAttr available as a member of TargetLibraryInfo to make it available easily.
  • Don't know how to handle OpenMP/OMPIRBuilder.cpp as there is no TargetLibraryInfo available. Furthermore, those functions are all declared in a .def file. It would have been nice to put in the extension attributes there, but that's not possible since the type of extension needs to be determined per target, right? Is there by any chance a known single type of extension used for all these function parameters, or is it needed to handle them one by one?
  • AddressSanitizer.cpp, MemorySanitizer.cpp, GCOVProfiling.cpp: These have a lot of missing attributes and I started adding as best I could, but not sure about all of them (sext or zext).

It's a little frustrating to have to fill all these out as it seems they really should only ever be used by clang, or? If so, maybe these functions could have something like an llvm-internal attribute so as to do the same trick as before by safely ignoring them?

Diff Detail

Event Timeline

jonpa created this revision.Sep 15 2022, 8:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2022, 8:48 AM
jonpa requested review of this revision.Sep 15 2022, 8:48 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: sstefan1. · View Herald Transcript
jonpa added a comment.Sep 22 2022, 1:42 AM

It's a little frustrating to have to fill all these out as it seems they really should only ever be used by clang, or? If so, maybe these functions could have something like an llvm-internal attribute so as to do the same trick as before by safely ignoring them?

At second thought, I think that would not work. The clang front end would add the attributes when building them from the .cpp files so the instrumented declarations really must hold them as well, right?

rnk edited reviewers, added: aeubanks; removed: rnk.Sep 22 2022, 2:18 PM

Don't know how to handle OpenMP/OMPIRBuilder.cpp as there is no TargetLibraryInfo available

Maybe split the computation out of TargetLibraryInfo, so we can use it without pulling in the entire analysis pass? The computation originated there because it's mostly needed by TargetLibraryInfo itself, but the ShouldExtI32Param/ShouldExtI32Return/ShouldSignExtI32Param bits aren't really tied to the rest of the computations done in TargetLibraryInfo.

Or we could just make the users of OMPIRBuilder construct a TargetLibraryInfo and pass it in, I guess.

At second thought, I think that would not work. The clang front end would add the attributes when building them from the .cpp files so the instrumented declarations really must hold them as well, right?

Yes, we're calling functions written in C++, so we need to follow the C/C++ calling convention.


getOrInsertFunction() is, in general, not guaranteed to return a Function. If opaque pointer types are enabled, it can't return a bitcast like it could before, but it could return function with the wrong number of arguments, or a global variable. Ideally, this shouldn't happen, but I wouldn't trust users not to do something weird.

It's probably more clear to pass in the attribute list like GCOVProfiling does. (In theory, we should then set attributes on the call, but I'm okay leaving that part out, at least for now.)

jonpa updated this revision to Diff 481956.EditedDec 11 2022, 4:05 PM

Getting back to this now finally, sorry for the delay.

It's probably more clear to pass in the attribute list like GCOVProfiling does...

I added a new method to TLI "getAttrList()" to quickly insert an AttributeList e.g. like

WarningFn = M.getOrInsertFunction("__msan_warning",
                                   TLI.getAttrList(C, {0}, false),
                                   IRB.getVoidTy(), IRB.getInt32Ty());

I have now gone through AddressSanitizer.cpp, MemorySanitizer.cpp, ThreadSanitizer.cpp, OMPKinds.def and GCOVProfiling.cpp, which all had many extension attributes missing.

Don't know much about the signedness of the different args, which of course is important to get correct. I guess I have used zero-extension mostly - it would be good to get this reviewed properly. In OMPKinds.def I added ZExt to all narrow int args, except SExt for args marked with /* Int */.

MemorySanitizer:createUserspaceApi(): Some arg ext attributes here are now added with getAttrList() instead, which means they are only added if the target requires it. Tests for this were therefore moved to SystemZ/vararg.ll

It's slightly confusing that these attributes are sometimes added unconditionally (as target is free to ignore them), but yet we sometimes use getExtAttrForI32Param() (directly or indirectly) to only add as requested. I have assumed that since there exists these target hooks guarding this, they should be used as much as possible and feasible. In OMPKinds.def for instance they are however added unconditionally.

Does this approach look ok? If so, I suppose I should continue with adding tests for the emitted attributes - some tests seem to be run only for X86 which may not get the attributes added as they would on SystemZ.

Don't know much about the signedness of the different args, which of course is important to get correct. I guess I have used zero-extension mostly - it would be good to get this reviewed properly. In OMPKinds.def I added ZExt to all narrow int args, except SExt for args marked with /* Int */.

You should simply follow the prototypes of the functions being called here. Any signed integer type (smaller than word size) needs SExt, any unsigned integer type (smaller than word size) needs ZExt.

For the OMP routines, those prototypes appear to be in openmp/runtime/src/kmp.h:

KMP_EXPORT void __kmpc_barrier(ident_t *, kmp_int32 global_tid);

where kmp_int32 is defined as int on all supported platforms in openmp/runtime/src/kmp_os.h, so it is a signed type.

MemorySanitizer:createUserspaceApi(): Some arg ext attributes here are now added with getAttrList() instead, which means they are only added if the target requires it. Tests for this were therefore moved to SystemZ/vararg.ll

This is right. We shouldn't be adding attributes unconditionally; at best, it's noise on targets where the attributes aren't significant.

llvm/include/llvm/Analysis/TargetLibraryInfo.h
437

This looks reasonable.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
2563

Usually we prefer to put the comment before the value. See https://llvm.org/docs/CodingStandards.html#comment-formatting

Probably also worth putting comments for the "signed" boolean.

jonpa updated this revision to Diff 484023.Dec 19 2022, 11:38 AM
jonpa marked 2 inline comments as done.

Spent some time now on adding all the extension attributes in these passes per previous discussions.

  • OpenMP

Don't know how to handle OpenMP/OMPIRBuilder.cpp as there is no TargetLibraryInfo available.

Maybe split the computation out of TargetLibraryInfo, so we can use it without pulling in the entire analysis pass? The computation originated there because it's mostly needed by TargetLibraryInfo itself, but the ShouldExtI32Param/ShouldExtI32Return/ShouldSignExtI32Param bits aren't really tied to the rest of the computations done in TargetLibraryInfo.

Or we could just make the users of OMPIRBuilder construct a TargetLibraryInfo and pass it in, I guess.

I found a solution where TargetLibraryInfo has both a static method taking the Triple, as well as the non-static method used with an initialized TLI object. Not sure this is ideal, but it at least works for both cases (don't know where to otherwise put these functions). Also wonder why those three variables have to be stored and copied around like that... Would it work to remove these variables and just have a function taking the Triple as argument?

Painstakingly gone through OMPKinds.def, after using as many declarations I could find (far from all in kmp.h). Adding SExt/ZExt for int/uint, and SizeTyExt for SizeTy, which extends SizeTy for <=i32 only (per our previous discussions with BuildLibCalls). Hopefully got them right, but unsure about these:

I could not find source declarations for:

__kmpc_get_hardware_num_blocks()       // __kmpc_get_hardware_num_threads_in_block looks similar and is returning uint
__tgt_target_data_begin_mapper_issue() // looked at hide_mem_transfer_latency.ll
__kmpc_is_generic_main_thread_id()     // guessing it's returning int8_t, like e.g. kmpc_is_spmd_exec_mode, guessing arg is int32_t
__tgt_target_data_begin_mapper_wait()  // No narrow int param, so no worries

These have mismatch in number of operands between source declaration and IR/OMPKinds.def, but I have not fixed in OMPKinds.def:

__tgt_target_data_begin_nowait_mapper()
__tgt_target_data_end_nowait_mapper()
__tgt_target_data_update_nowait_mapper()
__kmpc_parallel_level()                     // Called in parallel_level_fold.ll without params..?

These did not match argument declarations:

__tgt_interop_init()    // wrong arg types - also updated OpenMPIRBuilder::createOMPInteropInit() and the test interop_irbuilder.cpp.
__kmpc_target_init(), __kmpc_target_deinit()  // recently changed to drop the last parameter compared to the C level declarations (?).

add_attributes.ll: Updated to test all declarations of functions part of OMPKinds.def / OMPIRBuilder.cpp. SizeTy declared as i64. For enums, I have assumed an underlying default type of 'int' (signed i32) (correct?).

OpenMPIRBuilder::addAttributes() fixed to add extension attributes only after checking with target.

  • AddressSanitizer

Added tests to cover the different declarations affected.
Removed the extensions I previously added of the AMDGPU specific intrinsics at bottom of AddressSanitizer::initializeCallbacks().

  • GCOVProfiling

Could not find any test covering __gcov_fork, but tested the others.

  • MemorySanitizer

Found no test containing msan_warning().
Tests generating
msan_warning_with_origin_noreturn() did not compile with mips (unsupported target..?), and same with msan_chain_origin(), msan_set_origin() and __msan_memset(). It seems this isn't a huge deal as the MIPS extension variant by itself is tested elsewhere, and there are tests for the normal extensions (with SystemZ).

  • ThreadSanitizer

Changed to signed extension for __tsan_atomicXX_load, due to

__tsan_atomic32 __tsan_atomic32_load(const volatile __tsan_atomic32 *a,
    __tsan_memory_order mo);

  typedef int __tsan_atomic32
  enum __tsan_memory_order

Similarly for:

void __tsan_atomic32_store(volatile __tsan_atomic32 *a, __tsan_atomic32 v,
    __tsan_memory_order mo);

__tsan_atomic32 __tsan_atomic32_fetch_add(volatile __tsan_atomic32 *a,
    __tsan_atomic32 v, __tsan_memory_order mo);

__tsan_atomic32 __tsan_atomic32_compare_exchange_val(
    volatile __tsan_atomic32 *a, __tsan_atomic32 c, __tsan_atomic32 v,
    __tsan_memory_order mo, __tsan_memory_order fail_mo);

void __tsan_atomic_thread_fence(__tsan_memory_order mo);
void __tsan_atomic_signal_fence(__tsan_memory_order mo);

I don't really know anything about the specific OMP function definitions; I'll assume you have it right. (If someone more familiar could check, that would be welcome.)

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
344

Can we mess with the way this works so that the "attributes" from OMPKinds.def aren't directly LLVM IR attributes? I think the current version has the right semantics, but using zext/sext as "placeholder" attributes is really confusing. (Maybe instead of writing AttributeSet constructors in OMPKinds.def, make the names from OMP_ATTRS_SET into an enum, and put the actual code to translate from those enums to LLVM IR attributes here.)

I could not find source declarations for:

kmpc_get_hardware_num_blocks() // kmpc_get_hardware_num_threads_in_block looks similar and is returning uint

We somehow dropped this one and stopped using it. It was introduced in https://reviews.llvm.org/D106390, @josemonsalve2 can you take a look to re-introduce it (separate review).
From the current functions we use, uint32 is the return type.

__tgt_target_data_begin_mapper_issue() // looked at hide_mem_transfer_latency.ll

The impl. is under review (waiting for me) https://reviews.llvm.org/D133832. All signed as far as I can see.

__kmpc_is_generic_main_thread_id() // guessing it's returning int8_t, like e.g. kmpc_is_spmd_exec_mode, guessing arg is int32_t

We reordered the runtime, @jhuber6 can you check if we still need this and the folding code?

__tgt_target_data_begin_mapper_wait() // No narrow int param, so no worries

same as the issue method above, I think.

__kmpc_parallel_level() // Called in parallel_level_fold.ll without params..?

@tianshilei1992 Can you check the uses and tests for this please.

tgt_target_data_begin_nowait_mapper()
tgt_target_data_end_nowait_mapper()
__tgt_target_data_update_nowait_mapper()

These were introduced w/o all arguments, copy-paste error based on the non-nowait versions. We'll just need to add the corresponding arguments (@jhuber6, @tianshilei1992, @josemonsalve2).

Are there other problems with the OpenMP functions we can help with?

llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
402

We should ensure the enum size is 32 explicitly but this looks like a nice bugfix.

jonpa added inline comments.Dec 20 2022, 7:45 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
344

It says at the top of OMPKinds.def "This file is under transition to OMP.td with TableGen code generation.", so perhaps we could follow the current use of AttributeSet constructors in this patch for now..?

Personally, I haven't thought about this as a problem, even though the whole #include scheme isn't exactly super readable to me. Would probably be easier to work with using TableGen...

__kmpc_is_generic_main_thread_id() // guessing it's returning int8_t, like e.g. kmpc_is_spmd_exec_mode, guessing arg is int32_t

We reordered the runtime, @jhuber6 can you check if we still need this and the folding code?

I don't think it's still needed, we used to have some special initialization code only the main thread did and this would fold out the conditional if we knew only the main thread executed it anyway. That was deleted at some point, I can remove this code since it's only present in OpenMPOpt now.

jonpa updated this revision to Diff 484286.Dec 20 2022, 8:31 AM

Thanks for review. Extension of __tgt_target_data_begin_mapper_issue arg changed to SExt. I will wait for you on the other points...

Are there other problems with the OpenMP functions we can help with?

It would be very nice if you went through OMPKinds.def / add_attributes.ll and reviewed the extensions. I am myself also not at all familiar with these functions and have merely done my best by looking carefully at the source declarations.

__kmpc_parallel_level() // Called in parallel_level_fold.ll without params..?

@tianshilei1992 Can you check the uses and tests for this please.

In our old device runtime __kmpc_parallel_level doesn't have any params, while the new one added two. We didn't update everything properly. I'll make a patch to correct it. Thanks for the information.

It sounds like

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
344

I don't think it's really related to whether we use TableGen; either way, it's still bits of C++ code, and the question is just whether we put those bits into the header, or in a switch statement in this function. But I won't push too hard for this.

357

} else {

I could not find source declarations for:

kmpc_get_hardware_num_blocks() // kmpc_get_hardware_num_threads_in_block looks similar and is returning uint

We somehow dropped this one and stopped using it. It was introduced in https://reviews.llvm.org/D106390, @josemonsalve2 can you take a look to re-introduce it (separate review).

@jdoerfert If I remember correctly the name of these functions were changed to be compliant with the rest of the interface. They were still being used as part of the deviceRTL common interface. However, the calls to these functions must have been removed later on in the modifications to the runtime. I am ok if they disappear, if they are not being called. I could also add the necessary information if you think we will use these functions in the future.

jonpa updated this revision to Diff 485546.Dec 28 2022, 2:24 PM
jonpa marked 2 inline comments as done.

Rebased.

  • Incorporated the recent change of sign extending i32 returns for RISCV.
  • OpenMP:

__kmpc_omp_taskwait_deps_51: also seems to have some args missing, comparing to kmp.h... The two first kmp_int32 are now SExt:ed with this patch.
kmpc_is_generic_main_thread_id: attributes / test removed after function was removed by 7ae3db6.

I don't think it's really related to whether we use TableGen; either way, it's still bits of C++ code, and the question is just whether we put those bits into the header, or in a switch statement in this function. But I won't push too hard for this.

If it's ok with you, it seems easier for me to wait with that change as it is not really originating in this patch.

jonpa added a comment.Jan 9 2023, 12:30 PM

ping - any opinion on how to best determine the right extension actions for each target? Currently TLI holds variables (ShouldExt.../ShouldSignExt...), but TLI is not available in OMPIRBuilder. As discussed earlier, one option would of course be to provide TLI in OMPIRBuilder, but the alternative that I have found is to have a static function instead that just takes the Triple for each query. I think that would simplify things a bit (if always used) and I imagine it shouldn't be any compile time concern. What do you think?

efriedma accepted this revision.Jan 18 2023, 11:06 AM

LGTM. This is clearly an improvement, even if there are still some gaps.

I'm okay with doing the recomputation based on the triple for places which aren't too performance sensitive.

This revision is now accepted and ready to land.Jan 18 2023, 11:06 AM
This revision was landed with ongoing or failed builds.Jan 18 2023, 4:30 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2023, 4:30 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript