Page MenuHomePhabricator

Hahnfeld (Jonas Hahnfeld)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 2 2015, 4:52 AM (220 w, 5 d)
RWTH Aachen University

Recent Activity

Fri, Jun 21

Hahnfeld added a comment to D63599: Fixed memory use-after-free problem..

The fixes look good overall, but I'd like approval from somebody more familiar with this part of the code.

Fri, Jun 21, 9:09 AM · Restricted Project

Thu, Jun 20

Hahnfeld added a comment to D63607: [DO NOT SUBMIT] [clang][driver] Prototype --driver-mode=fortran support for new flang.

Even though I know you didn't send the email yet, can you please upload the diff with full context? Thanks :-)

Thu, Jun 20, 11:14 PM · Restricted Project
Hahnfeld added inline comments to D63599: Fixed memory use-after-free problem..
Thu, Jun 20, 8:47 AM · Restricted Project

Mon, Jun 17

Hahnfeld added inline comments to D63454: [OpenMP] Strengthen regression tests for task allocation under nowait depend clauses NFC.
Mon, Jun 17, 1:03 PM · Restricted Project, Restricted Project

Sat, Jun 15

Hahnfeld added a comment to D63009: [OpenMP] Add target task alloc function with device ID.

Am I correct that the second to last revision ("- Fix tests.") removed all checks for the actual device_id argument from the tests? From my point of view that's not fixing but weakening the tests! Can you explain why they needed "fixing"?

Sat, Jun 15, 12:33 AM · Restricted Project, Restricted Project, Restricted Project

Wed, Jun 12

Hahnfeld added a comment to D62393: [OPENMP][NVPTX]Mark parallel level counter as volatile..

Trying to catch up on all the comments, replies to some:

Wed, Jun 12, 8:16 AM · Restricted Project
Hahnfeld added reviewers for D63010: [OpenMP] Add task alloc function: jlpeyton, AndreyChurbanov.
Wed, Jun 12, 7:41 AM · Restricted Project, Restricted Project, Restricted Project

Mon, Jun 10

Hahnfeld added a comment to D63009: [OpenMP] Add target task alloc function with device ID.

As for D63010, the new function isn't used anywhere (and hence cannot be tested). What's the advantage of splitting changes at such granularity?

Mon, Jun 10, 4:49 AM · Restricted Project, Restricted Project, Restricted Project
Hahnfeld added a comment to D63010: [OpenMP] Add task alloc function.

Is there a particular reason for this kind of micro-patches? The function is not made available in the linker script and there's no implementation either.

Mon, Jun 10, 4:49 AM · Restricted Project, Restricted Project, Restricted Project
Hahnfeld added a comment to D60972: [OpenMP 5.0] libomptarget interface for declare mapper functions.

@Hahnfeld Do you really think it is necessary to pass these two functions as arguments, instead of exporting them. If you do, could you explain why?

Mon, Jun 10, 4:41 AM · Restricted Project

Tue, Jun 4

Hahnfeld added a comment to D62841: [openmp] Use libffi only when LLVM_ENABLE_FFI is on.

@Hahnfeld I'm not an OpenMP expert and don't have much strong argument here, but IMHO it might be confusing to have LLVM_ENABLE_FFI and LIBOMPTARGET_ENABLE_FFI have different default value from user's perspective. I wonder if we can set it off by default and explicitly turn it on for builds with test?

Tue, Jun 4, 11:48 PM · Restricted Project
Hahnfeld added a comment to D60972: [OpenMP 5.0] libomptarget interface for declare mapper functions.

The compiler doesn't generate code related to std::vector. It's only used in the runtime implementation, so it should be okay with Fortran. Again, the IBM and Intel compiler people seem to agree with it.

Maybe I don't understand where rt_mapper_handle comes from. According to the design slides and D59474, it passed as an argument to the generated omp_mapper[...] function, but how is the runtime system involved in its creation? Will there be additional interface functions / changes that will call this?

The idea is the runtime will create a MapperComponentsTy (std::vector) before calling the mapper function, in, for instance, __tgt_target_data_begin (These parts will be implemented in later patches). When the mapper function is called, the pointer of MapperComponentsTy is passed to it, as void *rt_mapper_handle. The mapper function will call __kmpc_push_mapper_component using this rt_mapper_handle, and then the runtime can put it into the MapperComponentsTy

Tue, Jun 4, 11:33 PM · Restricted Project
Hahnfeld added a comment to D62841: [openmp] Use libffi only when LLVM_ENABLE_FFI is on.

Oh, LLVM_ENABLE_FFI is off by default? That's not good, because we rely on the plugins to test libomptarget. So changing the default will have a negative impact on test coverage. I'm afraid that's not a good idea, even if we are now using libffi when LLVM_ENABLE_FFI = Off. We could still add LIBOMPTARGET_ENABLE_FFI (at the top-level CMakeLists.txt, please) and default to on, but I'm not sure if that's of much help.

Tue, Jun 4, 11:20 PM · Restricted Project
Hahnfeld added a comment to D60972: [OpenMP 5.0] libomptarget interface for declare mapper functions.

It would be great to have such things in public...

Sure, there is no secret. Please see it here if you are interested: https://github.com/lingda-li/public-sharing/blob/master/mapper_runtime_design.pptx

From a quick look, I'd say this does not reflect the current design: The types are named differently, have a different layout (SoA vs AoS) and there's no implementation of __tgt_target_mapper in this patch as @grokos mentioned.

Hi Jonas, starting from slide 8 is the current design, __tgt_target_mapper etc. are deprecated. It may not accurately reflect the current code but the framework should be the same.

Moreover, I'd question the following things:

  1. Why are we back to __kmpc_? naming? Most other functions specific to libomptarget are called __tgt_?.

There are other functions in libomptarget starting with __kmpc_, for example, __kmpc_push_target_tripcount.
My understanding is anything that does not directly call the device starting with __kmpc_. The IBM and Intel compiler people seem to be okay with this naming.

Tue, Jun 4, 7:24 AM · Restricted Project
Hahnfeld added a comment to D60972: [OpenMP 5.0] libomptarget interface for declare mapper functions.

It would be great to have such things in public...

Sure, there is no secret. Please see it here if you are interested: https://github.com/lingda-li/public-sharing/blob/master/mapper_runtime_design.pptx

Tue, Jun 4, 6:29 AM · Restricted Project

Mon, Jun 3

Hahnfeld added a comment to D60972: [OpenMP 5.0] libomptarget interface for declare mapper functions.

I think this patch is wrong. Where are the previous additions of target_data and __tgt_target_data_mapper? Possibly you uploaded the diff between the previous revision and the current one, whereas you need to upload the diff between origin/master and your current HEAD!

Please see the attachment of Ravi sent for the biweekly meeting to see details about this new scheme. Sorry I forgot to mention

Mon, Jun 3, 11:33 PM · Restricted Project
Hahnfeld requested changes to D62841: [openmp] Use libffi only when LLVM_ENABLE_FFI is on.

We support standalone builds of openmp where LLVM_ENABLE_FFI will not be defined. Can you add a check for OPENMP_STANDALONE_BUILD OR or DEFINED LLVM_ENABLE_FFI? Also please try to be consistent with the surrounding style, ie no whitespace after the opening and before the closing parenthesis.

Mon, Jun 3, 11:30 PM · Restricted Project

Tue, May 28

Hahnfeld added a comment to D61770: [OpenMP][AArch64] Fix compile with LLVM trunk..

Sorry, I didn't realize that's how the moderation works; subscribed now.

Tue, May 28, 9:25 AM · Restricted Project

May 26 2019

Hahnfeld added a comment to D61770: [OpenMP][AArch64] Fix compile with LLVM trunk..

Please subscribe to openmp-commits for future patches to the OpenMP runtime such that your revisions from Phabricator and more importantly commit emails get through.

May 26 2019, 8:20 AM · Restricted Project
Hahnfeld added a comment to D62199: [OPENMP][NVPTX]Fix barriers and parallel level counters, NFC..

Alexey, I believe that I understand the distinction that you are making. That's not the way the term is used within the LLVM community. The observable behavior of the compiler and its runtime libraries is what matters. The fact that some implementation technique should not be required to make that observable behavior correct is irrelevant in our usage. The introduction of that technique in order to make the observable behavior correct, where it was not previously correct, is a functionality-changing change to the compiler. Thus, it is not NFC.

It is better to express this explicitly in the policy, because it differs from the classical definition.

May 26 2019, 12:47 AM · Restricted Project, Restricted Project

May 25 2019

Hahnfeld added a comment to D60884: [OpenMP][libomptarget] Enable usage of unified memory for declare target link variables.

Looks mostly good, one last question about the return type of __tgt_rtl_init_requires that I noticed only now.

May 25 2019, 5:08 AM · Restricted Project, Restricted Project

May 22 2019

Hahnfeld added a comment to D62199: [OPENMP][NVPTX]Fix barriers and parallel level counters, NFC..
  1. Because it is impossible to test them, that why it is NFC.

This doesn't make it NFC. There is intended change in behavior.

May 22 2019, 11:52 PM · Restricted Project, Restricted Project
Hahnfeld added a comment to D45890: [OMPT] Add implementation and tests of Archer tool.

Some high-level comments inline

May 22 2019, 1:03 AM

May 21 2019

Hahnfeld added inline comments to D60884: [OpenMP][libomptarget] Enable usage of unified memory for declare target link variables.
May 21 2019, 11:58 AM · Restricted Project, Restricted Project
Hahnfeld added a comment to D60223: [OpenMP][libomptarget] Enable requires flags for target libraries..

LG in case we don't need __tgt_register_requires to return a code in the near future (like for saying "that's not supported")

May 21 2019, 11:53 AM · Restricted Project
Hahnfeld accepted D60223: [OpenMP][libomptarget] Enable requires flags for target libraries..

LG in case we don't need __tgt_register_requires to return a code in the near future (like for saying "that's not supported")

May 21 2019, 11:49 AM · Restricted Project
Hahnfeld added a comment to D60223: [OpenMP][libomptarget] Enable requires flags for target libraries..

In this patch __tgt_rtl_init_requires does nothing. Do we really want to add the prototype now? If yes, are we sure that we won't need to pass more information in the future? Once we add it, the runtime library must maintain compatibility.

May 21 2019, 8:39 AM · Restricted Project

May 18 2019

Hahnfeld added a comment to D60223: [OpenMP][libomptarget] Enable requires flags for target libraries..

In this patch __tgt_rtl_init_requires does nothing. Do we really want to add the prototype now? If yes, are we sure that we won't need to pass more information in the future? Once we add it, the runtime library must maintain compatibility.

May 18 2019, 6:52 AM · Restricted Project

May 15 2019

Hahnfeld added inline comments to D61949: [OpenMP][bugfix] Fix issues with C++ 17 compilation when handling math functions.
May 15 2019, 8:38 AM · Restricted Project

May 8 2019

Hahnfeld added a comment to D61657: Add implementation to two OMPT API routines.

If possible we should also have a test for ompt_finalize_tool.

May 8 2019, 12:51 AM · Restricted Project

May 6 2019

Hahnfeld added inline comments to D61603: [OpenMP] Fix gfortran bugzilla build bug 41755.
May 6 2019, 12:12 PM · Restricted Project, Restricted Project

May 1 2019

Hahnfeld added a comment to D61380: [OPENMP][NVPTX]Fix behavior of omp_in_parallel() function..

My understanding of enclosed is that its meaning is recursive. So omp_in_parallel is to return true iff (at least) one of the surrounding parallel regions is active. That also matches the summary in OpenMP 5.0 which says

The omp_in_parallel routine returns true if the active-levels-var ICV is greater than zero; otherwise, it returns false.

and

active-levels-var - the number of nested active parallel regions that enclose the current task such that all of the parallel regions are enclosed by the outermost initial task region on the current device. There is one copy of this ICV per data environment.

May 1 2019, 8:00 AM · Restricted Project

Apr 26 2019

Hahnfeld added a comment to D42642: [CUDA] Detect installation in PATH.

Just a quick heads-up - the cuda-detect-path.cu test fails on WSL/Ubuntu 18.04:

Apr 26 2019, 6:53 AM · Restricted Project

Apr 20 2019

Hahnfeld added a comment to D60907: [OpenMP] Add math functions support in OpenMP offloading.

So the scheme is: pow is defined in __clang_openmp_math.h to call __kmpc_pow. This lives in libomptarget-nvptx (both bc and static lib) and just calls pow which works because nvcc and Clang in CUDA mode make sure that the call gets routed into libdevice?

Apr 20 2019, 12:31 AM · Restricted Project
Hahnfeld added a comment to D60918: [OPENMP][NVPTX]Correctly handle L2 parallelism in SPMD mode..

Why is it enough to have one counter per warp, what happens if threads within a warp diverge? Before D55773 we had a counter per thread...

Apr 20 2019, 12:20 AM · Restricted Project, Restricted Project

Apr 15 2019

Hahnfeld added a comment to D60578: [OPENMP][NVPTX]Fix dynamic scheduling in L2+ SPMD parallel regions..

So, the test is good or I should put it into some other directory?

@Hahnfeld , libomptarget/deviceRTLs/nvptx/test/parallel works for you for now, correct?

Apr 15 2019, 12:57 PM · Restricted Project
Hahnfeld added a comment to D60578: [OPENMP][NVPTX]Fix dynamic scheduling in L2+ SPMD parallel regions..

In that case these tests need to be marked UNSUPPORTED for versions of Clang that will not pass them. There's infrastructure for that, but it's not applied in the current form of this patch.

Okay. This patch review is not the right place to discuss the libomptarget support for old Clang versions. We should have a separate thread on this subject.

In any case, let's move forward with adding this test in that directory, and then we'll address the infrastructure issue as follow-up work.

So put differently, you're proposing to land this in its current form (which will break for some users, including me) and wait for "somebody" to work on the infrastructure to fix things?

Of course I'm not. I'm proposing that we put the test in the nvptx directory and then address the fact that it should apply to other offloading targets as follow up.

Apr 15 2019, 11:44 AM · Restricted Project
Hahnfeld added a comment to D60578: [OPENMP][NVPTX]Fix dynamic scheduling in L2+ SPMD parallel regions..

What check command do you run to run these nvptx tests?

Apr 15 2019, 1:18 AM · Restricted Project

Apr 13 2019

Hahnfeld added a comment to D60578: [OPENMP][NVPTX]Fix dynamic scheduling in L2+ SPMD parallel regions..

I think we need to be careful about adding nvptx tests to libomptarget/test: They can be executed using Clang later than 6.0.0, but that version wasn't able to offload to GPUs. Given that the changes are limited to libomptarget-nvptx (because its parallelism is kind of special), I think the new test should go to libomptarget/deviceRTLs/nvptx/test. Just my 2 cents...

I don't see anything in this test that is nvptx specific. Is there something about the semantics that make it specific to nvptx? We need to build of a suite of tests for accelerator offloading in general. We'll have other accelerator backends (e.g., for AMD GPUs), and the offloading tests should apply to them too.

Apr 13 2019, 6:04 AM · Restricted Project
Hahnfeld added a comment to D60578: [OPENMP][NVPTX]Fix dynamic scheduling in L2+ SPMD parallel regions..

Our general policy is that all commits that can have tests, should have tests. We have OpenMP target tests in libomptarget/test -- and given that you've added tests there yourself, I assume that you know this ;) -- plus tests in libomptarget/deviceRTLs/nvptx/test - although it sounds like this situation can be triggered using portable code, so I'd prefer we add a test in libomptarget/test. Can you please do that?

Sure, if we have a testing infrastructure for this, I'll add the test. Just missed the tests for NVPTX, will definitely add it.

Apr 13 2019, 2:02 AM · Restricted Project

Apr 11 2019

Hahnfeld added a comment to D60578: [OPENMP][NVPTX]Fix dynamic scheduling in L2+ SPMD parallel regions..

Why don't we have unit tests here or in the llvm-test suite?

Because this is the library. Do you have an idea how to write the unit tests for it? It can be tested only with the executable tests. I know, that someone worked on the target-based testsuite, but don't know when it is going to be ready.

Apr 11 2019, 11:22 PM · Restricted Project

Apr 5 2019

Hahnfeld added inline comments to D59451: Fix gettid warnings and one test on FreeBSD.
Apr 5 2019, 2:13 PM · Restricted Project, Restricted Project

Apr 2 2019

Hahnfeld accepted D59451: Fix gettid warnings and one test on FreeBSD.

AFAICS all comments were addressed and this looks good. Please wait a day or so in case I missed something

Apr 2 2019, 2:02 PM · Restricted Project, Restricted Project
Hahnfeld accepted D60114: [CMake] Differentiate between static and shared libc++abi.
Apr 2 2019, 1:52 PM · Restricted Project
Hahnfeld added a comment to D60114: [CMake] Differentiate between static and shared libc++abi.

I guess that change makes sense overall, some comments from reading the code inline. I didn't apply the patch locally so I might be missing some context here...

Apr 2 2019, 5:41 AM · Restricted Project
Hahnfeld added inline comments to D59451: Fix gettid warnings and one test on FreeBSD.
Apr 2 2019, 5:31 AM · Restricted Project, Restricted Project

Mar 27 2019

Hahnfeld added a comment to D59880: [OpenMP][WIP] RISCV64 port.

Most of the changes look pretty straight forward to me, but I currently don't have time to read through the assembly implementation of __kmp_invoke_microtask. I left two comments inline to discuss compressed vs uncompressed encodings for testing OMPT.

Mar 27 2019, 8:20 AM · Restricted Project

Mar 24 2019

Hahnfeld requested changes to D59451: Fix gettid warnings and one test on FreeBSD.

The code in cmake/OpenMPTesting.cmake should also handle standalone builds of openmp.

Mar 24 2019, 9:49 AM · Restricted Project, Restricted Project

Mar 17 2019

Hahnfeld updated subscribers of D59451: Fix gettid warnings and one test on FreeBSD.

I'm not sure about OMP specifics, but in C++ threads we are allowed to use threading routines but in order to make them functional we must link final executable with libpthread.

On NetBSD threading is in a separate library out of libc.

So if a developer uses a feature from stdlib, they need to manually link libpthread? I always thought that the C++ standard guarantees you a fully working stdlib out of the box, but might be wrong here.

Mar 17 2019, 5:31 AM · Restricted Project, Restricted Project
Hahnfeld added a comment to D59451: Fix gettid warnings and one test on FreeBSD.

I'm not sure about OMP specifics, but in C++ threads we are allowed to use threading routines but in order to make them functional we must link final executable with libpthread.

On NetBSD threading is in a separate library out of libc.

Mar 17 2019, 4:42 AM · Restricted Project, Restricted Project
Hahnfeld added a comment to D59451: Fix gettid warnings and one test on FreeBSD.
In D59451#1432251, @dim wrote:

This code does not directly use pthreads, but uses C++11 threads.
Why does the compiler not link the necessary threading library, when using C++11 threads?

How would the compiler know up-front what is in the code? I have not often seen compilers that influence linking flags from the code that was compiled, unless maybe the Microsoft specific #pragma lib feature (which somebody is now adding to clang, I believe). Usually, these kinds of flags are specified by the user, and passed from the frontend to the linker.

Mar 17 2019, 4:36 AM · Restricted Project, Restricted Project
Hahnfeld updated subscribers of D57571: [clang-tidy] A new OpenMP module.
Mar 17 2019, 4:30 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Mar 15 2019

Hahnfeld committed rG77eccf24d5c9: [msan] Fix BMI2 detection in msan tests, take 2. (authored by Hahnfeld).
[msan] Fix BMI2 detection in msan tests, take 2.
Mar 15 2019, 3:19 AM
Hahnfeld committed rCRT356242: [msan] Fix BMI2 detection in msan tests, take 2..
[msan] Fix BMI2 detection in msan tests, take 2.
Mar 15 2019, 3:19 AM
Hahnfeld committed rL356242: [msan] Fix BMI2 detection in msan tests, take 2..
[msan] Fix BMI2 detection in msan tests, take 2.
Mar 15 2019, 3:19 AM
Hahnfeld added a comment to D58858: [msan] Instrument x86 BMI intrinsics..

Okay, the test failure is related to my Intel Westmere system (yeah, that's old, I know) where I see:

b = 0x13c0
kBmi12Mask = 0x108

Even the "corrected" code only checked that at least one bit is set :-(

Mar 15 2019, 3:19 AM · Restricted Project, Restricted Project

Mar 13 2019

Hahnfeld added a comment to D45691: [mips] Use libatomic instead of GCC intrinsics for 64bit.

Please note this change set broke the following buildbot and has yet to be fixed: http://lab.llvm.org:8011/builders/openmp-clang-ppc64le-linux-rhel/builds/140

Mar 13 2019, 10:22 AM · Restricted Project
Hahnfeld added a comment to D58858: [msan] Instrument x86 BMI intrinsics..

I'm running that configuration for years and it always worked. Do you have a change at hand that did such compiler change?

Do you run msan lit tests? This is an example where new compiler behavior is tested the same change list, less than a year ago:
https://reviews.llvm.org/rL332761

Mar 13 2019, 3:53 AM · Restricted Project, Restricted Project
Hahnfeld committed rGc64d73cce240: [ELF] Fix GCC8 warnings about "fall through", NFCI (authored by Hahnfeld).
[ELF] Fix GCC8 warnings about "fall through", NFCI
Mar 13 2019, 3:39 AM
Hahnfeld committed rGe59746f8f823: [Support] Treat truncation of fullpath as error (authored by Hahnfeld).
[Support] Treat truncation of fullpath as error
Mar 13 2019, 3:39 AM
Hahnfeld committed rL356037: [ELF] Fix GCC8 warnings about "fall through", NFCI.
[ELF] Fix GCC8 warnings about "fall through", NFCI
Mar 13 2019, 3:38 AM
Hahnfeld closed D58837: [ELF] Fix GCC8 warnings about "fall through", NFCI.
Mar 13 2019, 3:38 AM · Restricted Project
Hahnfeld committed rL356036: [Support] Treat truncation of fullpath as error.
[Support] Treat truncation of fullpath as error
Mar 13 2019, 3:38 AM
Hahnfeld closed D58835: [Support] Treat truncation of fullpath as error.
Mar 13 2019, 3:38 AM · Restricted Project

Mar 12 2019

Herald added a project to D55772: [OpenMP][libomptarget] Suppress C++ 11 related warnings when building libomptarget-nvptx bitcode library: Restricted Project.

Hi Doru, I think this patch is not yet in trunk? Additionally I'm seeing the same warnings when compiling with nvcc which doesn't seem addressed here. Should we pass -std=c++11 when compiling the CUDA code as well?

Mar 12 2019, 4:23 AM · Restricted Project
Hahnfeld added inline comments to D58837: [ELF] Fix GCC8 warnings about "fall through", NFCI.
Mar 12 2019, 4:15 AM · Restricted Project
Hahnfeld added a comment to D58837: [ELF] Fix GCC8 warnings about "fall through", NFCI.

Gentle ping

Mar 12 2019, 2:57 AM · Restricted Project
Hahnfeld added a comment to D58835: [Support] Treat truncation of fullpath as error.

Ping. As I said I think the current approach with a fixed-size buffer makes sense because anything larger likely won't work anyhow.

Mar 12 2019, 2:57 AM · Restricted Project
Hahnfeld added a comment to D58858: [msan] Instrument x86 BMI intrinsics..

This is expected to break. I'm surprised it did not break earlier - we often do a compiler change and corresponding integration tests in compiler-rt as a single commit, or two consecutive commits.

Mar 12 2019, 2:00 AM · Restricted Project, Restricted Project

Mar 11 2019

Hahnfeld added a comment to D58959: [DebugInfo][ExecutionEngine] follow up for "add SectionedAddress to DebugInfo interfaces".

I think the change is correct, but I don't know enough of the code to feel confident.

Mar 11 2019, 3:08 AM · Restricted Project

Mar 10 2019

Hahnfeld added a comment to D58858: [msan] Instrument x86 BMI intrinsics..

The added test is failing for me because I'm building (and testing) compiler-rt with Clang 7 which doesn't instrument BMI, and I'd assume the same with Clang 8 once released. Is that an oversight or is this configuration expected to break?

Mar 10 2019, 4:31 AM · Restricted Project, Restricted Project

Mar 6 2019

Hahnfeld added a comment to D58801: [Support] Implement is_local_impl with AIX mntctl.

Ping. I am looking for some input on whether we want to pursue additional testing.

Mar 6 2019, 11:33 AM · Restricted Project

Mar 5 2019

Hahnfeld added inline comments to D58959: [DebugInfo][ExecutionEngine] follow up for "add SectionedAddress to DebugInfo interfaces".
Mar 5 2019, 10:08 AM · Restricted Project

Mar 4 2019

Hahnfeld committed rG65a401f6a90f: [AArch64/ARM] Fix two compiler warnings in InstructionSelector, NFCI (authored by Hahnfeld).
[AArch64/ARM] Fix two compiler warnings in InstructionSelector, NFCI
Mar 4 2019, 12:51 AM
Hahnfeld committed rL355304: [AArch64/ARM] Fix two compiler warnings in InstructionSelector, NFCI.
[AArch64/ARM] Fix two compiler warnings in InstructionSelector, NFCI
Mar 4 2019, 12:51 AM
Hahnfeld closed D58834: [AArch64/ARM] Fix two compiler warnings in InstructionSelector, NFCI.
Mar 4 2019, 12:50 AM · Restricted Project

Mar 3 2019

Hahnfeld added inline comments to D58835: [Support] Treat truncation of fullpath as error.
Mar 3 2019, 12:44 PM · Restricted Project
Hahnfeld added inline comments to D58837: [ELF] Fix GCC8 warnings about "fall through", NFCI.
Mar 3 2019, 7:11 AM · Restricted Project
Hahnfeld updated the diff for D58837: [ELF] Fix GCC8 warnings about "fall through", NFCI.

Move common ELF description to Inputs/.

Mar 3 2019, 7:11 AM · Restricted Project
Hahnfeld added inline comments to D58837: [ELF] Fix GCC8 warnings about "fall through", NFCI.
Mar 3 2019, 4:44 AM · Restricted Project

Mar 2 2019

Hahnfeld requested review of D58837: [ELF] Fix GCC8 warnings about "fall through", NFCI.

I think we could just add a break and handle that case in the generic switch below. But I think you're right, the current patch would break for Hexagon, MIPS and PPC64. I tested with assertions enabled, but maybe there is no test case exercising this code path?

Yes. For this patch using break; should be good enough. There may not be tests for such corrupted binaries.

Mar 2 2019, 7:54 AM · Restricted Project
Hahnfeld updated the diff for D58837: [ELF] Fix GCC8 warnings about "fall through", NFCI.

Replace llvm_unreachables in Object/ELF.cpp by break statements and add test to exercise this code path.

Mar 2 2019, 7:54 AM · Restricted Project
Hahnfeld added a comment to D58837: [ELF] Fix GCC8 warnings about "fall through", NFCI.

getDynamicTagAsString is used by llvm-objdump and it'd be better if llvm-objdump can parse some malformed binaries (with bad .dynsym). If the tag is unknown, falling back to the default label (return "<unknown:>0x"...) may make more sense.

Mar 2 2019, 12:41 AM · Restricted Project

Mar 1 2019

Hahnfeld created D58837: [ELF] Fix GCC8 warnings about "fall through", NFCI.
Mar 1 2019, 10:29 AM · Restricted Project
Hahnfeld created D58835: [Support] Treat truncation of fullpath as error.
Mar 1 2019, 10:26 AM · Restricted Project
Hahnfeld created D58834: [AArch64/ARM] Fix two compiler warnings in InstructionSelector, NFCI.
Mar 1 2019, 10:22 AM · Restricted Project
Hahnfeld committed rGe071cd86dfc7: Hide two unused debugging methods, NFCI. (authored by Hahnfeld).
Hide two unused debugging methods, NFCI.
Mar 1 2019, 9:17 AM
Hahnfeld committed rL355205: Hide two unused debugging methods, NFCI..
Hide two unused debugging methods, NFCI.
Mar 1 2019, 9:14 AM
Hahnfeld added a comment to D57986: [ProfileData] Sort FuncData before iteration to remove non-determinism.

I think this patch is right in also sorting the function names: AFAICS StringMap doesn't provide that guarantee.

Mar 1 2019, 3:59 AM · Restricted Project, Restricted Project
Hahnfeld added a comment to D56286: [OPENMP] Deal with additional store inserted by Clang under -fno-PIC for PowerPC..

Is this STW instruction something that instruction scheduling might move? (@nemanjai ?)

I am fairly certain that the STW is the store of the value returned by the function so the scheduler should leave it alone. Perhaps @stefanp can verify this.

Mar 1 2019, 3:31 AM · Restricted Project, Restricted Project
Hahnfeld added inline comments to D58533: [hwasan, asan] Intercept vfork..
Mar 1 2019, 3:27 AM · Restricted Project, Restricted Project
Hahnfeld abandoned D58787: [ProfileData] Sort ProfilingData by hash.
Mar 1 2019, 1:21 AM · Restricted Project
Hahnfeld abandoned D58385: [tools] Rewrite tests for symbol remapping to FileCheck.
Mar 1 2019, 1:21 AM · Restricted Project

Feb 28 2019

Hahnfeld added a comment to D58787: [ProfileData] Sort ProfilingData by hash.

I don't care how this is solved, but it needs to be solved! And it's not getting easier with everyone saying something different

Feb 28 2019, 10:49 AM · Restricted Project
Hahnfeld added a comment to D58787: [ProfileData] Sort ProfilingData by hash.

The secondary map 'ProfileData' is needed occasionally (to use cfg hash). doing sorting with std::map is not needed strictly speaking, but it has the nice property of having fixed order.

Feb 28 2019, 9:17 AM · Restricted Project
Hahnfeld added a comment to D58787: [ProfileData] Sort ProfilingData by hash.

Have you considered only imposing the ordering/sorting where it matters,
i.e. i think where FunctionData is used in InstrProfWriter::writeImpl() and InstrProfWriter::writeText()?

Feb 28 2019, 9:08 AM · Restricted Project
Hahnfeld added a comment to D58385: [tools] Rewrite tests for symbol remapping to FileCheck.

Another approach would be to sort the output and make it independent from reverse-iteration. See D58631 for how this was done for ThinLTO. I'm fine with whatever unbreaks the tests, but I'm not familiar with the code so "fixing" the tests and getting review on this was easier for me.

Feb 28 2019, 8:41 AM · Restricted Project
Hahnfeld created D58787: [ProfileData] Sort ProfilingData by hash.
Feb 28 2019, 8:40 AM · Restricted Project
Hahnfeld added inline comments to D58533: [hwasan, asan] Intercept vfork..
Feb 28 2019, 6:52 AM · Restricted Project, Restricted Project

Feb 26 2019

Hahnfeld added a comment to D58385: [tools] Rewrite tests for symbol remapping to FileCheck.

Ping, that's now the only test that fails the reverse-iteration bot...

This seems to only shuffle the tests around.
Does this now pass in reverse-iteration config?

Yes, I've tested this locally. The idea for instr-remap.test is to invoke FileCheck three times, checking each function separately.

So then i think i'm missing the point..

If it currently passes without reverse iteration, and does not pass with reverse iteration,
then that means there is some nondeterminism, correct?

Feb 26 2019, 6:40 AM · Restricted Project