Page MenuHomePhabricator

jdoerfert (Johannes Doerfert)
Argonne National Laboratory

Projects

User does not belong to any projects.

User Details

User Since
Jan 30 2014, 6:27 AM (363 w, 3 d)

Recent Activity

Yesterday

jdoerfert added a reviewer for D94884: [Clang][OpenMP] Include header for CUDA builtin vars into OpenMP wrapper header: tra.

I can see that we want this but I guess the errors show the problem, replace __attribute((device)) with DEVICE and define it based on CUDA vs OpenMP properly.

Sun, Jan 17, 1:18 PM · Restricted Project
jdoerfert added a comment to D94871: [Clang][OpenMP] Fixed an issue that clang crashed when compiling OpenMP program in device only mode without host IR.

Here is a minimal reproducer for the issue even without standalone device compilation (and I can build the situation in various other ways as well): https://godbolt.org/z/T1h9b5
Can you add it as a test, I am also curious how the IR looks like with this patch.

Sun, Jan 17, 1:15 PM · Restricted Project, Restricted Project

Fri, Jan 15

jdoerfert added inline comments to D94743: [Attributor][FIX] Do not delete non`-mustprogress` calls.
Fri, Jan 15, 12:31 PM · Restricted Project
jdoerfert accepted D94806: [OpenMP] Add support for mapping names in mapper API.

LGTM

Fri, Jan 15, 10:22 AM · Restricted Project, Restricted Project, Restricted Project
jdoerfert accepted D94180: [SimplifyCFG] Optimize CFG when null is passed to a function with nonnull argument.

Generally LGTM, assuming you are fine with my comments. If you want to continue the discussion, we can as well.

Fri, Jan 15, 10:19 AM · Restricted Project
jdoerfert updated the diff for D94743: [Attributor][FIX] Do not delete non`-mustprogress` calls.

Add more mustprogress/willreturn recursion tests

Fri, Jan 15, 8:45 AM · Restricted Project
jdoerfert added inline comments to D94743: [Attributor][FIX] Do not delete non`-mustprogress` calls.
Fri, Jan 15, 7:51 AM · Restricted Project
jdoerfert added a comment to D94687: [AArch64] Make target intrinsics DefaultAttrIntrinsics..

(Some ARM-aware ppl should review/accept)

Fri, Jan 15, 7:10 AM · Restricted Project
jdoerfert added a comment to D94106: [Local] Treat calls that may not return as being alive (WIP)..

FTR, this is the "corresponding" Attributor patch: D94743

Fri, Jan 15, 7:09 AM · Restricted Project

Thu, Jan 14

jdoerfert added a comment to D94744: [Attributor][NFC] Make `--allow-unused-prefixes=true` explicit in the tests.

LGTM, but I only looked at lit.local.cfg.

Thu, Jan 14, 8:34 PM · Restricted Project
jdoerfert added a comment to D93078: [utils] Fix UpdateTestChecks case where 2 runs differ for last label.

I'm not sure how this is more helpful. What is the use case where this way of warning helps?

For tests other than attributor, that explicitly set FileCheck --allow-unused-prefixes=true, these warnings mean that there will be unused prefixes (those listed)

Should not we check for that flag in the RUN line then and only warn for unused prefixes when it is set. If there is no prefix we should obviously always warn.

That's a good idea. Probably we'd need to also make sure that the unused prefixes are all on RUN lines with --allow-unused-prefixes=true.

I'm also not sure how lit.local.cfg interacts with the test prefix updater: currently, the only cases where we bulk-want to allow unused prefixes is the Attributor tests. If you were going to add the flag explicitly, that'd also work. Or just the option to the update_test_prefix that says "ok with duplicates, don't warn"

I can add the option explicitly (D94744). We should look for the filecheck one if possible, two options means double the hassle. That said, why are we warning in both FileCheck and update_test_check, it seems to be unnecessary to do the latter.

update_test_prefix.py's role is temporary: once we flip FileCheck to disallow unused prefixes by default, we don't need to keep it around. At that point, it becomes important for the update_<xyz>_test_check scripts to warn.

Thu, Jan 14, 8:33 PM · Restricted Project, Restricted Project
jdoerfert added inline comments to D90103: Add OpenMP for optimization.
Thu, Jan 14, 8:23 PM · Restricted Project, Restricted Project
jdoerfert added a comment to D93078: [utils] Fix UpdateTestChecks case where 2 runs differ for last label.

I'm not sure how this is more helpful. What is the use case where this way of warning helps?

For tests other than attributor, that explicitly set FileCheck --allow-unused-prefixes=true, these warnings mean that there will be unused prefixes (those listed)

Should not we check for that flag in the RUN line then and only warn for unused prefixes when it is set. If there is no prefix we should obviously always warn.

That's a good idea. Probably we'd need to also make sure that the unused prefixes are all on RUN lines with --allow-unused-prefixes=true.

I'm also not sure how lit.local.cfg interacts with the test prefix updater: currently, the only cases where we bulk-want to allow unused prefixes is the Attributor tests. If you were going to add the flag explicitly, that'd also work. Or just the option to the update_test_prefix that says "ok with duplicates, don't warn"

Thu, Jan 14, 8:16 PM · Restricted Project, Restricted Project
jdoerfert requested review of D94744: [Attributor][NFC] Make `--allow-unused-prefixes=true` explicit in the tests.
Thu, Jan 14, 8:14 PM · Restricted Project
jdoerfert requested review of D94743: [Attributor][FIX] Do not delete non`-mustprogress` calls.
Thu, Jan 14, 8:01 PM · Restricted Project
jdoerfert added a reviewer for D94740: [Attributor] Create `AAMustProgress` for the `mustprogress` attribute: atmnpatel.
Thu, Jan 14, 8:01 PM · Restricted Project
jdoerfert added a comment to D93078: [utils] Fix UpdateTestChecks case where 2 runs differ for last label.

I think this caused a lot of problems for the Attributor tests. I get the "conflict output" warning all the time now :(

These two UpdateTestChecks tests also generate the warning. I would think that they haven't before.

LLVM :: tools/UpdateTestChecks/update_test_checks/check_attrs.test
LLVM :: tools/UpdateTestChecks/update_test_checks/prefix-never-matches.test

I suspect the --check-attributes flag effect are not taken into account somewhere but I might be wrong.

The warnings are correct, but their purpose is to inform. For example, take check_attrs. The third and fourth opt run produce different function attributes from those produced by the first two, so the function is different insofar as update_test_prefix is concerned. The warnings are there to indicate that the listed prefixes end up not being used. But, that's 'by design' for the attributor tests, IIRC.

If the warnings are noise for you, we could add a flag to quiet them down, which you'd flip when regenerating attributor tests?

TBH, before, the warnings meant there is a problem that needs fixing. Now they mean, there might be one or not. So, depending on your setup it's just noise while it was pure signal before.
I'm not sure how this is more helpful. What is the use case where this way of warning helps?

For tests other than attributor, that explicitly set FileCheck --allow-unused-prefixes=true, these warnings mean that there will be unused prefixes (those listed)

Thu, Jan 14, 7:25 PM · Restricted Project, Restricted Project
jdoerfert requested review of D94741: [Utils][WIP] Check for more global information in update_test_checks.
Thu, Jan 14, 7:20 PM · Restricted Project
jdoerfert requested review of D94740: [Attributor] Create `AAMustProgress` for the `mustprogress` attribute.
Thu, Jan 14, 7:13 PM · Restricted Project
jdoerfert added a comment to D93078: [utils] Fix UpdateTestChecks case where 2 runs differ for last label.

I think this caused a lot of problems for the Attributor tests. I get the "conflict output" warning all the time now :(

These two UpdateTestChecks tests also generate the warning. I would think that they haven't before.

LLVM :: tools/UpdateTestChecks/update_test_checks/check_attrs.test
LLVM :: tools/UpdateTestChecks/update_test_checks/prefix-never-matches.test

I suspect the --check-attributes flag effect are not taken into account somewhere but I might be wrong.

The warnings are correct, but their purpose is to inform. For example, take check_attrs. The third and fourth opt run produce different function attributes from those produced by the first two, so the function is different insofar as update_test_prefix is concerned. The warnings are there to indicate that the listed prefixes end up not being used. But, that's 'by design' for the attributor tests, IIRC.

If the warnings are noise for you, we could add a flag to quiet them down, which you'd flip when regenerating attributor tests?

Thu, Jan 14, 7:09 PM · Restricted Project, Restricted Project
jdoerfert accepted D94731: [libomptarget][nvptx] Reduce calls to cuda header.

LGTM.

Thu, Jan 14, 6:10 PM · Restricted Project
jdoerfert added a comment to D6594: Fix for bug 8281 - Extremely slow assembling and disassembling of ptrtoint.

Could you add the test as well. Also, include the context in the patch. See https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

Thu, Jan 14, 5:18 PM · Restricted Project
jdoerfert added a comment to D94731: [libomptarget][nvptx] Reduce calls to cuda header.

I don't have a solution to unknown CUDA_VERSION yet.

Thu, Jan 14, 5:12 PM · Restricted Project
jdoerfert added a comment to D94731: [libomptarget][nvptx] Reduce calls to cuda header.

I don't really like that we have to implement the cuda functions we already implement in places like clang/lib/Headers/__clang_cuda_device_functions.h.
Another problem is we still depend on the CUDA_VERSION and thereby cuda.h. We should forward declare what we need here and put it in the unconditionally included openmp_wrapper headers.

Thu, Jan 14, 4:48 PM · Restricted Project
jdoerfert reopened D93078: [utils] Fix UpdateTestChecks case where 2 runs differ for last label.

I think this caused a lot of problems for the Attributor tests. I get the "conflict output" warning all the time now :(

Thu, Jan 14, 3:17 PM · Restricted Project, Restricted Project
jdoerfert added inline comments to D94684: [InferAttrs] Mark some library functions as willreturn..
Thu, Jan 14, 1:51 PM · Restricted Project
jdoerfert added a comment to D94633: [FunctionAttrs] Infer willreturn for functions without loops.

EDIT: I'm fine with this patch, FWIW.

I still think we should check to enable a light-attributor run in the beginning of the pipeline.

I was hoping to get the willreturn-related bugs fixed in time for LLVM 12, so sticking to the existing infrastructure seems like the safer bet :) It probably makes more sense to enable the attributor after branching rather than before.

Fair. As I said, we can enable it in a restricted, lightweight mode first. Could you remind me which bugs?

D94106 fixes the important one, though isGuaranteedToTransferExecutionToSuccessor is also buggy for languages without forward progress. Functions with infinite loops getting optimized away is the number one Rust miscompile by quite a margin...

Thu, Jan 14, 1:48 PM · Restricted Project
jdoerfert added a comment to D94700: [OpenMP] Dropped unnecessary define when compiling deviceRTLs for NVPTX.

Sure. But don't get too distracted here. All of the nvcc stuff will go away.

We still need some target specific code in any case. If we can eventually get rid of CUDA headers, then this could be useless. :-)

Thu, Jan 14, 10:18 AM · Restricted Project
jdoerfert accepted D94700: [OpenMP] Dropped unnecessary define when compiling deviceRTLs for NVPTX.

Sure. But don't get too distracted here. All of the nvcc stuff will go away.

Thu, Jan 14, 10:09 AM · Restricted Project
jdoerfert added a comment to D94633: [FunctionAttrs] Infer willreturn for functions without loops.

I still think we should check to enable a light-attributor run in the beginning of the pipeline.

I was hoping to get the willreturn-related bugs fixed in time for LLVM 12, so sticking to the existing infrastructure seems like the safer bet :) It probably makes more sense to enable the attributor after branching rather than before.

Thu, Jan 14, 10:06 AM · Restricted Project
jdoerfert added inline comments to D94698: [OpenMP][WIP][POC] Compile the device runtime as C++.
Thu, Jan 14, 9:24 AM · Restricted Project
jdoerfert updated the diff for D94698: [OpenMP][WIP][POC] Compile the device runtime as C++.

Remove accidentially included header

Thu, Jan 14, 9:20 AM · Restricted Project
jdoerfert requested review of D94698: [OpenMP][WIP][POC] Compile the device runtime as C++.
Thu, Jan 14, 9:17 AM · Restricted Project
jdoerfert added a comment to D93974: [ValueTracking] Safe assumption context for args.

Oh.. this is bad. That happens if you mix two logically differnet things into a single instruction pointer.
I put it on the list of things that need to fixed wrt. assumes ... :(

Excellent. Is the list published anywhere?

Thu, Jan 14, 8:41 AM · Restricted Project
jdoerfert added a comment to D94633: [FunctionAttrs] Infer willreturn for functions without loops.

I still think we should check to enable a light-attributor run in the beginning of the pipeline. The logic seems sound, could be improved since this does initialize the fixpoint computation with a pessimistic starting value. One comment below, the rest is OK for merging.

Thu, Jan 14, 8:25 AM · Restricted Project
jdoerfert added a comment to D94684: [InferAttrs] Mark some library functions as willreturn..

Looks ok.

We also miss noundef on many libcalls, don’t we?

Thu, Jan 14, 7:58 AM · Restricted Project
jdoerfert updated subscribers of D94687: [AArch64] Make target intrinsics DefaultAttrIntrinsics..

Some notable exceptions I think are exclusive loads and stores as well as the memory barrier intrinsics, for which nosync does not apply I think.

Thu, Jan 14, 7:55 AM · Restricted Project

Wed, Jan 13

jdoerfert added a comment to D93946: [FuncAttrs] Infer noreturn.

Not terribly important but isn't

A function is noreturn if all blocks terminating with a ReturnInst contain a call to a noreturn function.

slightly in conflict with the Lint pass containing the following "Unusual" warning?

Wed, Jan 13, 8:34 AM · Restricted Project
jdoerfert accepted D94562: [OpenMP] Add documentation for error messages and release notes.

LGTM

Wed, Jan 13, 7:59 AM · Restricted Project

Tue, Jan 12

jdoerfert accepted D94573: [OpenMP] Drop the static library libomptarget-nvptx.

Nice

Tue, Jan 12, 9:48 PM · Restricted Project
jdoerfert added a comment to D93974: [ValueTracking] Safe assumption context for args.
define dso_local i32 @_Z1fi(i32 %0) local_unnamed_addr #0 {
  %2 = and i32 %0, 3
  %3 = icmp eq i32 %2, 0
  tail call void @llvm.assume(i1 %3)
  %4 = or i32 %0, 1
  ret i32 %4
}

Computing the known bits of %0 with %2 as the context would have isValidAssumeForContext() return false on the assume since %2 is one of its ephemeral values. Otherwise, simplifying %2 by computing %0's known bits would be logical bootstrapping - %0's assumed zero bits would be used to simplify %2 to zero, then %3 to true and the assume would be lost.

Tue, Jan 12, 1:59 PM · Restricted Project
jdoerfert accepted D93764: [LoopUnswitch] Implement first version of partial unswitching..

I think I got it now. Due to the PHI nodes we can follow uses and eventually visit every definition in the loop (we care about). LGTM. Feel free to wait for someone else to look at it, or not.

Tue, Jan 12, 12:15 PM · Restricted Project
jdoerfert accepted D94534: [OpenMP] Fixed include directories for OpenMP when building OpenMP with LLVM_ENABLE_RUNTIMES.

LGTM

Tue, Jan 12, 11:06 AM · Restricted Project
jdoerfert added a comment to D93974: [ValueTracking] Safe assumption context for args.

Yes, if isaAssumeIntrinsic stands for "is an assume or an ephemeral of an assume".

Tue, Jan 12, 10:46 AM · Restricted Project
jdoerfert added a comment to D93946: [FuncAttrs] Infer noreturn.

Hi folks, I'm afraid an 8% regression in h264ref (SPEC INT 2006) bisects to this patch. This in on AArch64 with -flto on a Neoverse-N1 based system, but I wouldn't be surprised if this showed up elsewhere too.

How do people feel about reverting the patch while we investigate the regression? To start with, I'll try to find out why this has caused the drop in performance: it doesn't seem immediately obvious to me.

I really can't see this change causing performance regressions. It only infers more precise function attributes. Could you do some initial investigation or come up with a repro? Maybe an optimization remark that doesn't fire after this change?

Tue, Jan 12, 10:02 AM · Restricted Project
jdoerfert added a comment to D93974: [ValueTracking] Safe assumption context for args.

The idea is that you can always do what you do here in isValidAssumeForContext (and friends). I just checked and we seem to do so already. Could you explain to me why we need to go for the Last instruction in this patch at all? What would happen if you simply pick the first in the entry block, which is trivially correct. (Note: You can skip llvm.assume to make some weird problems go away).

The patch indeed defaults to the first instruction in the entry. Scanning to the end at safeCxtI() was an optimization for cases where the first instruction is an ephemeral value of an assume (which seems quite likely for assumes about arguments) that would get the assume discarded by the isEphemeralValueOf(Inv, CxtI) check. At isValidAssumeForContext() we can't distinguish between a context given only as a control-flow marker and a context that also guards against simplifying an ephemeral so we can't try to improve it there, but since any context safeCxtI() provides for a null CxtI is just a control-flow marker anyway we might as well choose one that's not an ephemeral of any assume in the entry block. Does that make sense?

Tue, Jan 12, 9:46 AM · Restricted Project
jdoerfert added inline comments to D94502: [FunctionAttrs] Derive willreturn for fns with readonly` & `mustprogress`..
Tue, Jan 12, 9:01 AM · Restricted Project
jdoerfert accepted D94502: [FunctionAttrs] Derive willreturn for fns with readonly` & `mustprogress`..

I left some comments that need to be addressed, not necessarily how I proposed it but somehow. LGTM otherwise.

Tue, Jan 12, 8:48 AM · Restricted Project
jdoerfert added a comment to D93974: [ValueTracking] Safe assumption context for args.

FWIW, I agree with @nikic, we should not put this logic here. There are two problems:

  1. We compute something we might not need.
  2. We do it only in value tracking.

Thanks for taking a look, Johannes!

I'd prefer the following, though the nullptr proposal seems fine too:
a) For arguments use the first instruction in the entry as context, this is trivial and correct.

Agreed. Will limit this patch for this low-hanging fruit. Handling null contexts in isValidAssumptionForContext() is indeed more general, but also seems to have greater potential for causing trivial simplification of ephemeral values. If it works out it would replace the safe context for arguments.

b) When the context is used, e.g., to look for assumption, allow to some exploration of surrounding instructions.

Not sure I see how (beyond the existing extension to reachable instructions). Since isValidAssumeForContext() can't tell why its context was chosen it must assume the context might be protecting an ephemeral from simplification, right?

Tue, Jan 12, 8:08 AM · Restricted Project
jdoerfert accepted D94386: [LangRef] State that a nocapture pointer cannot be returned.

This spells out uncontroversial effective semantics, so LGTM.

Tue, Jan 12, 7:55 AM · Restricted Project
jdoerfert accepted D90529: Allow nonnull/align attribute to accept poison.

For the attributor, add the tests that don't work with FIXMEs into the attributor test folder please, potentially just into the respective test files, e.g. nonnull.ll

Tue, Jan 12, 7:52 AM · Restricted Project

Mon, Jan 11

jdoerfert accepted D93039: Introduce llvm.noalias.decl intrinsic.

A few minor wording suggestions and comments, nothing that would require another round of review. Adopt my suggestions as you see fit, add TODOs where it makes sense and something should be investigated in the future. LGTM

Mon, Jan 11, 3:19 PM · Restricted Project
jdoerfert added a comment to D94443: [OpenMP] Take elf_common.c as a interface library.

Thanks for this.

Mon, Jan 11, 2:42 PM · Restricted Project
jdoerfert added inline comments to D93764: [LoopUnswitch] Implement first version of partial unswitching..
Mon, Jan 11, 2:28 PM · Restricted Project
jdoerfert added a comment to D94433: [ValueTracking] Check that alignment is non-zero in computeKnownBitsFromAssume (PR48713)..

No doubt there is a problem here somewhere, not sure if this is the best fix though or just hides it one level deeper.

Mon, Jan 11, 11:57 AM · Restricted Project
jdoerfert added a comment to D93764: [LoopUnswitch] Implement first version of partial unswitching..

Generally fine, I have one questions though

Mon, Jan 11, 10:59 AM · Restricted Project
jdoerfert added a comment to D93189: Introduce the `!nocapture` metadata and "nocapture_use" operand bundle.

Other people have input on this?

Mon, Jan 11, 9:07 AM · Restricted Project
jdoerfert added inline comments to D94125: [Attributor] Derive `willreturn` based on `mustprogress`.
Mon, Jan 11, 7:26 AM · Restricted Project
jdoerfert accepted D94379: [OpenMP] Move memory manager to plugin and make it a common interface.

I forgot to submit some nits yesterday, address what makes sense ^. LGTM

Mon, Jan 11, 6:53 AM · Restricted Project
jdoerfert added inline comments to D94379: [OpenMP] Move memory manager to plugin and make it a common interface.
Mon, Jan 11, 6:53 AM · Restricted Project
jdoerfert added a comment to D94386: [LangRef] State that a nocapture pointer cannot be returned.

Should we add the, "if broken -> UB!" clause while we are here?

Mon, Jan 11, 6:51 AM · Restricted Project

Sun, Jan 10

jdoerfert added a comment to D94379: [OpenMP] Move memory manager to plugin and make it a common interface.

This looks better.

Sun, Jan 10, 5:21 PM · Restricted Project
jdoerfert added a comment to D82703: [InstCombine] convert assumes to operand bundles.

@Tyker are you going to upstream this? If not, can someone take over?

Sun, Jan 10, 2:22 PM · Restricted Project
jdoerfert added a comment to D93974: [ValueTracking] Safe assumption context for args.

FWIW, I agree with @nikic, we should not put this logic here. There are two problems:

  1. We compute something we might not need.
  2. We do it only in value tracking.
Sun, Jan 10, 2:21 PM · Restricted Project
jdoerfert added reviewers for D94337: Add cuda header type for cuh files: tra, jlebar.
Sun, Jan 10, 2:01 PM · Restricted Project
jdoerfert retitled D94241: [OpenMP] Fix hierarchical barrier from Fix hierarchical barrier to [OpenMP] Fix hierarchical barrier.
Sun, Jan 10, 1:44 PM · Restricted Project
jdoerfert accepted D93738: [OpenMP] Not set OPENMP_STANDALONE_BUILD=ON when building OpenMP along with LLVM.

LGTM, and should work for the runtime tests.

I could not figure out, why clang-resource-headers was there in the first place. As I understand clang-resource-headers, it's just an install target?
Also, the relevant files behind this target seem to be target-specific wrappers of headers like math.h and new. They might be relevant for some libomptarget tests? But, grep didn't show any use of math headers in libomptarget tests.

Sun, Jan 10, 1:42 PM · Restricted Project, Restricted Project
jdoerfert added inline comments to D89671: [LLVM][OpenMP] Adding support for OpenMP sections construct in OpenMPIRBuilder.
Sun, Jan 10, 1:40 PM · Restricted Project, Restricted Project
jdoerfert accepted D94369: [LLVM] Added OpenMP to `LLVM_ALL_RUNTIMES`.

LGTM, please add a commit message explaining what this does though.

Sun, Jan 10, 1:38 PM · Restricted Project
jdoerfert accepted D94373: [LLVM][OpenMP] Added OMP Scheduler Constants (Required for D89671).

LGTM, one comment that should be addressed.

Sun, Jan 10, 1:33 PM · Restricted Project

Fri, Jan 8

jdoerfert added inline comments to D93734: [LoopDeletion] Insert an early exit from dead path in loop.
Fri, Jan 8, 2:33 PM · Restricted Project
jdoerfert added a comment to D94315: [OpenMP][FIX] Enforce a function boundary for a new data environment.

What we should do, as we move to the OpenMPIRBuilder, is to use runtime interfaces that match OpenMP directives.
Here is the omp parallel one for the host: D94332
The device one should look the same, potentially after extending it, as we want to apply the same logic regardless of the device.
This will also make things like parallel region merging (@ggeorgakoudis) much easier.

I would prefer to fix PR48686 with this patch though until we switch to a new interface. The part where I remove the fn argument
meddling is on it's own valuable. We should not (need to) overwrite the function attributes for performance reasons given that we
have not inserted them. The new noinline will establish the invariant D94332 is going to have as well and which makes OpenMPOpt
much simpler.

It would be better to teach OpenMPOpt about current limitations, because these changes may prevent some optimizations, but if this is hard to implement/something else, better to have a bugfix rather than doing something the user does not expect.

Fri, Jan 8, 2:17 PM · Restricted Project
jdoerfert updated subscribers of D94315: [OpenMP][FIX] Enforce a function boundary for a new data environment.

What we should do, as we move to the OpenMPIRBuilder, is to use runtime interfaces that match OpenMP directives.
Here is the omp parallel one for the host: D94332
The device one should look the same, potentially after extending it, as we want to apply the same logic regardless of the device.
This will also make things like parallel region merging (@ggeorgakoudis) much easier.

Fri, Jan 8, 12:38 PM · Restricted Project
jdoerfert requested review of D94332: [OpenMP] Introduce a new parallel region entry point.
Fri, Jan 8, 12:33 PM · Restricted Project
jdoerfert added a comment to D94315: [OpenMP][FIX] Enforce a function boundary for a new data environment.

I don't understand where inaccessiblemem_or_argmemonly is coming from. This prevents us from inlining the outlined parallel region.

kmpc_serialized_parallel and kmpc_end_serialized_parallel functions are marked with this attribute and I think OpenMPOpt optimizes read_only functions because of this. I.e. it does not consider kmpc_serialized_parallel and kmpc_end_serialized_paralle function calls as opimization barriers.

Can you provide an example why this is a problem?

Looks like PR48686, you mentioned, caused by this. The second function call should not be optimized, because it is inside the region between kmpc_serialized_parallel() and kmpc_end_serialized_parallel() function calls.

Fri, Jan 8, 11:00 AM · Restricted Project
jdoerfert added a comment to D94315: [OpenMP][FIX] Enforce a function boundary for a new data environment.

I don't understand where inaccessiblemem_or_argmemonly is coming from. This prevents us from inlining the outlined parallel region.

kmpc_serialized_parallel and kmpc_end_serialized_parallel functions are marked with this attribute and I think OpenMPOpt optimizes read_only functions because of this. I.e. it does not consider kmpc_serialized_parallel and kmpc_end_serialized_paralle function calls as opimization barriers.

Fri, Jan 8, 10:12 AM · Restricted Project
jdoerfert added a comment to D94315: [OpenMP][FIX] Enforce a function boundary for a new data environment.

I'm guessing we're using the function boundary as a compiler barrier. That seems fragile in the face of improving cross-function optimisation.

Looks like applying inaccessiblemem_or_argmemonly attribute to the OpenMP functions is way too optimistic since we still can access this (inaccessible) memory using other OpenMP functions. Not sure about the semantics of this attribute, though.

Fri, Jan 8, 10:05 AM · Restricted Project
jdoerfert requested review of D94315: [OpenMP][FIX] Enforce a function boundary for a new data environment.
Fri, Jan 8, 8:33 AM · Restricted Project
jdoerfert added inline comments to D93189: Introduce the `!nocapture` metadata and "nocapture_use" operand bundle.
Fri, Jan 8, 7:50 AM · Restricted Project

Thu, Jan 7

jdoerfert updated the diff for D93189: Introduce the `!nocapture` metadata and "nocapture_use" operand bundle.

Address comments and improve lang ref

Thu, Jan 7, 5:08 PM · Restricted Project
jdoerfert added a comment to D93189: Introduce the `!nocapture` metadata and "nocapture_use" operand bundle.

I have a question - is it possible to have the same optimization power with !nocapture only? In other words, is it purely for making analysis flow-insensitive?

Thu, Jan 7, 5:05 PM · Restricted Project
jdoerfert updated the summary of D93189: Introduce the `!nocapture` metadata and "nocapture_use" operand bundle.
Thu, Jan 7, 4:21 PM · Restricted Project
jdoerfert updated the diff for D93189: Introduce the `!nocapture` metadata and "nocapture_use" operand bundle.

Add !nocapture to the store instruction syntax

Thu, Jan 7, 4:10 PM · Restricted Project
jdoerfert updated the diff for D93189: Introduce the `!nocapture` metadata and "nocapture_use" operand bundle.

Actually remove the OpenMP piece

Thu, Jan 7, 4:04 PM · Restricted Project
jdoerfert retitled D93189: Introduce the `!nocapture` metadata and "nocapture_use" operand bundle from [WIP] Introduce the `!nocapture` metadata and "nocapture_use" operand bundle to Introduce the `!nocapture` metadata and "nocapture_use" operand bundle.
Thu, Jan 7, 4:03 PM · Restricted Project
jdoerfert updated the diff for D93189: Introduce the `!nocapture` metadata and "nocapture_use" operand bundle.

Add tests and "nocapture_use" handling. Split off OpenMP usage.

Thu, Jan 7, 4:01 PM · Restricted Project
jdoerfert accepted D94263: [OpenMP] Always print error messages in libomptarget CUDA plugin.

LGTM

Thu, Jan 7, 2:18 PM · Restricted Project
jdoerfert committed rG0b0f2e6ee0c3: [OpenMP][FIX] Avoid string literal comparison, use `StringRef::equal` (authored by jdoerfert).
[OpenMP][FIX] Avoid string literal comparison, use `StringRef::equal`
Thu, Jan 7, 12:54 PM
jdoerfert added a comment to D94256: [OpenMP] Fixed the issue that memory manager calls plugin interface to release buffered memory even after the plugin has been destroyed.

One nit, works for me otherwise. Anyone else?

Thu, Jan 7, 12:51 PM · Restricted Project
jdoerfert added inline comments to D94256: [OpenMP] Fixed the issue that memory manager calls plugin interface to release buffered memory even after the plugin has been destroyed.
Thu, Jan 7, 12:42 PM · Restricted Project
jdoerfert committed rG6e7101530dae: [OpenMP][Docs] Mark finished features as done (authored by jdoerfert).
[OpenMP][Docs] Mark finished features as done
Thu, Jan 7, 12:39 PM
jdoerfert closed D94185: [OpenMP][Docs] Mark finished features as done.
Thu, Jan 7, 12:39 PM · Restricted Project
jdoerfert committed rG36c4dc9b42fe: [OpenMP][FIX] Ensure the isa trait is evaluated last (authored by jdoerfert).
[OpenMP][FIX] Ensure the isa trait is evaluated last
Thu, Jan 7, 12:32 PM
jdoerfert committed rGd970a285b856: [OpenMP][Fix] Make the arch selector for x86_64 work (authored by jdoerfert).
[OpenMP][Fix] Make the arch selector for x86_64 work
Thu, Jan 7, 12:32 PM
jdoerfert closed D93785: [OpenMP][FIX] Ensure the isa trait is evaluated last.
Thu, Jan 7, 12:32 PM · Restricted Project, Restricted Project
jdoerfert committed rG9ae171bcd38c: [OpenMP][Docs] Add remarks intro section (authored by jdoerfert).
[OpenMP][Docs] Add remarks intro section
Thu, Jan 7, 12:31 PM
jdoerfert closed D93786: [OpenMP][Fix] Make the arch selector for x86_64 work.
Thu, Jan 7, 12:31 PM · Restricted Project, Restricted Project
jdoerfert closed D93735: [OpenMP][Docs] Add remarks intro section.
Thu, Jan 7, 12:31 PM · Restricted Project
jdoerfert added a comment to D86844: [LoopDeletion] Allows deletion of possibly infinite side-effect free loops.
int a, b;

int f(void) {
    while (1) {
        if (a != b) return 1;
    }
    return 0;
}

int g(int a, int b) {
    while (1) {
        if (a != b) return 1;
    }
    return 0;
}

LLVM does not catch these cases; gcc does.

https://godbolt.org/z/jW7son

Looks like must progress does not get added? If it gets added to the IR the loops get removed: https://godbolt.org/z/77v17P

I might be misunderstanding the standard here but since 1 is a non-zero constant expression, it can't be assumed to terminate by the implementation right? The relevant section from C11 at least is "An iteration statement whose controlling expression is not a constant expression that performs [explanation of what it deems as progress] may be assumed by the implementation to terminate" (C11 6.8.5 p6). I think these cases fall outside of the scope of this particular change ...

For C yes, but are there such rules for C++? GCC in c++ mode does not check for non-zero constant expr and happily performs this optimization.

Thu, Jan 7, 12:24 PM · Restricted Project, Restricted Project
jdoerfert added a comment to D91944: OpenMP 5.0 metadirective.

Are the test all passing?

Thu, Jan 7, 12:23 PM · Restricted Project, Restricted Project, Restricted Project