This is an archive of the discontinued LLVM Phabricator instance.

[NVPTX] Enable lowering of atomics on local memory
ClosedPublic

Authored by wsmoses on Mar 15 2021, 11:49 AM.

Details

Summary

LLVM does not have valid assembly backends for atomicrmw on local memory. However, as this memory is thread local, we should be able to lower this to the relevant load/store.

Diff Detail

Event Timeline

wsmoses created this revision.Mar 15 2021, 11:49 AM
wsmoses requested review of this revision.Mar 15 2021, 11:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2021, 11:49 AM
tra added a reviewer: tra.Mar 15 2021, 12:11 PM
tra added a subscriber: tra.

I think the right place to deal with this is during the lowering. This pass just works around the actual problem.

llvm/lib/Target/NVPTX/NVPTXAtomicLower.cpp
80

The description needs to be updated.

wsmoses updated this revision to Diff 330835.Mar 15 2021, 4:20 PM

Address comment regarding description

No test.

llvm/lib/Target/NVPTX/NVPTXAtomicLower.cpp
71

V is not a descriptive name.
auto* where appropriate.
if the type is not clean, don't use auto.
Why only for monotonic orderings?
Why only FAdd?
Don't we have an existing way to "deconstruct" atomics intriniscs into loads/stores?

wsmoses updated this revision to Diff 340182.Apr 23 2021, 3:28 PM
wsmoses marked an inline comment as done.

Add test, rename variables, and use existing atomic lower code

Only minor nits from me, @tra, you should review this I suppose.

llvm/lib/Target/NVPTX/NVPTXAtomicLower.cpp
45
54

Nit: for (Instructions &I : instructions(F))
I would omit the braces, also below.

tra accepted this revision.Apr 26 2021, 10:22 AM

Few nits. LGTM overall.

llvm/include/llvm/Transforms/Scalar/LowerAtomic.h
17

We could probably get by with a class AtomicRMWInst;. We don't need the whole header just for the pointer type.

32

Nit: function names should be camel case, and start with a lower case letter (e.g. openFile() or isFoo())., according to LLVM style guide: https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

llvm/lib/Target/NVPTX/NVPTXAtomicLower.cpp
64–66

llvm::initializeNVPTXAtomicLowerPass(PassRegistry &); would do.

llvm/test/CodeGen/NVPTX/atomic-lower.ll
7–12

The file should be renamed to atomic-lower-local.ll and there should be a comment describing that the test ensures that we're able to lower atomics in local AS in principle and that in case of NVPTX we happen to lower them to regular load/stores. Without the explanation, the CHECK lines by themselves are somewhat puzzling as there is nothing about atomics in the IR they check for.

This revision is now accepted and ready to land.Apr 26 2021, 10:22 AM
wsmoses updated this revision to Diff 340614.Apr 26 2021, 12:51 PM

Address comments

This revision was landed with ongoing or failed builds.Apr 26 2021, 4:28 PM
This revision was automatically updated to reflect the committed changes.