[PATCH] D133949: Make sure the right parameter extension attributes are added in various instrumentation passes.

Mon Dec 19 11:38:38 PST 2022

jonpa updated this revision to Diff 484023.
jonpa marked 2 inline comments as done.
jonpa added a comment.
Herald added subscribers: pcwang-thead, s.egerton, atanasyan, jrtc27, simoncook, fedor.sergeev, sdardis.

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);

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

https://reviews.llvm.org/D133949

Files:
  clang/test/OpenMP/interop_irbuilder.cpp
  llvm/include/llvm/Analysis/TargetLibraryInfo.h
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/lib/Analysis/TargetLibraryInfo.cpp
  llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
  llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
  llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp
  llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
  llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
  llvm/test/Instrumentation/AddressSanitizer/experiment.ll
  llvm/test/Instrumentation/AddressSanitizer/mem-intrinsics.ll
  llvm/test/Instrumentation/MemorySanitizer/Mips/vararg-mips64.ll
  llvm/test/Instrumentation/MemorySanitizer/SystemZ/vararg.ll
  llvm/test/Instrumentation/MemorySanitizer/atomics.ll
  llvm/test/Instrumentation/MemorySanitizer/msan_basic.ll
  llvm/test/Instrumentation/ThreadSanitizer/atomic.ll
  llvm/test/Transforms/GCOVProfiling/function-numbering.ll
  llvm/test/Transforms/OpenMP/add_attributes.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D133949.484023.patch
Type: text/x-patch
Size: 134909 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20221219/1433445a/attachment-0001.bin>