Eliminate call target for intrinsics in Subzero [179497998]

Feature Request

Status Update

No update yet.

Description

ni...@google.com

created issue #1

Feb 5, 2021 07:08PM

Subzero's InstIntrinsicCall is derived from InstCall, and the latter takes a reference to the call's 'target' (a symbolic function pointer). InstIntrinsicsCall supports this too, but it's actually one used once, for a 64-bit atomic operation used for profiling.

The target parameter gets in the way, because it's stored as the first source parameter of the instruction (this makes it easier to handle symbol relocations), which makes intrinsics while look like loads and stores (specifically LoadSubVector and StoreSubVector) have their parameters stored at different indexes than built-in load and store instructions. This in turn causes all optimization passes which attempt to optimize loads and stores to have to discern between the two to get the correct parameters.

We can avoid that by eliminating the target parameter. This means either rewriting the profiler's use of it (it might not be necessary, TargetX86Base<TraitsType>::genTargetHelperCallFor() doesn't produce calls for atomic operations, they're handled using cmpxchg and instructions with a lock prefix), or removing profiling support altogether. We haven't used Subzero's profiling functionality thus far, and I doubt we'd have a use for it in the future since we can insert atomic counters at the Reactor level or above when we need to collect such data and VTune can't help us out.

Note that the alternative of deriving an InstLoadSubVector from InstLoad instead (i.e. defining it as a new core instruction instead of an intrinsic), and the equivalent for sub-vector store, has major complications because code which handles InstLoad or InstStore assumes vector types are stored in memory in their entirety.

Comments

ni...@google.com <ni...@google.com> #2Feb 10, 2021 01:19AM

One design decision worth elaboring on is whether or not to rename InstIntrinsicsCall to InstIntrinsics. With InstIntrinsicsCall no longer derived from InstCall, to get rid of the Target parameter, it can never get lowered into a call (at least not a call for which the target is unknown to the backend). In high-level languages, intrinsics look like function calls, but at Subzero's IR level they look more like instructions. Hence dropping the "Call" from the name would make sense. Alternatives discussed with Antonio include:

Keep the InstCall target operand separate from the Srcs operand list, so the goal of aligning the source operands of load/store-like intrinsics with those of regular load/store instructions is still achieved. Unfortunately this causes complications for liveness analysis, since the target operand of a regular call can be a function pointer produced by other instructions which need to be considered live. We could teach Subzero to treat the target operand as an additional source operand, but it's a bit risky and might require more than liveness analysis to be updated.
Move the target operand to the last position of Srcs[]. This works without affecting things like liveness analysis, but is still a bit risky since we must ensure nothing looks for the target operand at index 0. It would also still be an unused undefined value for intrinsics.
Add virtual methods for accessing load/store operands. Currently there are no virtual methods for Inst, and since we process many thousands of these it could have a performance impact to make virtual calls. Also note it already uses a Kind enum to perform its own RTTI.

It's worth noting that to LLVM, intrinsics are definitely considered functions: https://llvm.org/docs/LangRef.html#intrinsic-functions. They can even only be invoked through a call instruction. Despite Subzero's design clearly having been inspired by LLVM, it uses a separate class for representing intrinsics. InstIntrinsicCall also takes an Intrinsics::IntrinsicInfo argument at construction, whereas with LLVM that information is part of the call target.

Hence I intend on moving forward with renaming it to InstIntrinsics, and adding some comments to clarify that it represents a extension instruction which few parts of the compiler have to know the exact functionality about.

ap...@google.com <ap...@google.com> #3Feb 10, 2021 05:47AM

Project: SwiftShader
Branch: master

commit 33a77f7f129311dcbb7e1480af812de2fe9177db
Author: Nicolas Capens <capn@google.com>
Date: Mon Feb 08 15:04:38 2021

Rename InstIntrinsicCall to InstIntrinsic

It is no longer derived from InstCall, and doesn't take a Target
parameter which could be a symbol to a function which implements
the intrinsic.

Note one can still emit actual Call instructions if a function call
is needed. Since the previous change which removed the Target parameter
we can no longer decide between implementing an intrinsic as inline
instructions or a function call at the Subzero level, but we can still
do that at the Reactor level which has its own concept of intrinsics.

This change also removes mentions of intrinsics representing function
calls. It also removes code related to PNaCl-specific LLVM intrinsics,
including the ability to look up intrinsics by name. The addArg(),
getArg(), and getNumArgs() methods, adopted from InstCall (but no longer
inherited from it), are kept for now due to risk of replacing the ones
for InstCall objects, while the confusion caused by keeping the
function-related "arg" term is deemed low.

Bug:

b/179497998

Change-Id: I293f039853abff6f5bebda1b714774205bdec846
Reviewed-on:

https://swiftshader-review.googlesource.com/c/SwiftShader/+/52608
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>

M src/Reactor/Optimizer.cpp
M src/Reactor/SubzeroReactor.cpp
M third_party/subzero/docs/DESIGN.rst
M third_party/subzero/src/IceCfg.cpp
M third_party/subzero/src/IceConverter.cpp
M third_party/subzero/src/IceELFObjectWriter.cpp
M third_party/subzero/src/IceGlobalContext.cpp
M third_party/subzero/src/IceGlobalContext.h
M third_party/subzero/src/IceGlobalInits.cpp
M third_party/subzero/src/IceGlobalInits.h
M third_party/subzero/src/IceInst.cpp
M third_party/subzero/src/IceInst.h
M third_party/subzero/src/IceInstrumentation.cpp
M third_party/subzero/src/IceInstrumentation.h
M third_party/subzero/src/IceIntrinsics.cpp
M third_party/subzero/src/IceIntrinsics.h
M third_party/subzero/src/IceTargetLowering.cpp
M third_party/subzero/src/IceTargetLowering.h
M third_party/subzero/src/IceTargetLoweringARM32.cpp
M third_party/subzero/src/IceTargetLoweringARM32.h
M third_party/subzero/src/IceTargetLoweringMIPS32.cpp
M third_party/subzero/src/IceTargetLoweringMIPS32.h
M third_party/subzero/src/IceTargetLoweringX86Base.h
M third_party/subzero/src/IceTargetLoweringX86BaseImpl.h
M third_party/subzero/src/PNaClTranslator.cpp
M third_party/subzero/src/WasmTranslator.cpp

https://swiftshader-review.googlesource.com/52608

ap...@google.com <ap...@google.com> #4Feb 10, 2021 05:47AM

Project: SwiftShader
Branch: master

commit 673a7fe5c3ff6686c98951cafb24a93ca5bd1303
Author: Nicolas Capens <capn@google.com>
Date: Fri Feb 05 15:03:22 2021

Unify load/store operand accessors

Load and store instructions, as well as intrinsics which access memory,
can now shared the same methods for accessing the memory address and
data operands.

Note that while this change introduces the potential for non-load/store
instructions to have their operands accessed through getLoadAddress(),
getStoreAddress(), or getData(), that risk isn't any greater than using
the wrong getSrc() index, and would stick out as a mistake much more
clearly. The advantage this change brings is that we no longer have to
remember where the address and data operands are stored in sub-vector
load/store intrinsics. In addition, there are no more overly verbose
casts, and their cost is eliminated.

Bug:

b/179497998

Change-Id: I0d9208555e00b0d3053f7d3baca241fef2b8cbeb
Reviewed-on:

https://swiftshader-review.googlesource.com/c/SwiftShader/+/52531
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>

M src/Reactor/Optimizer.cpp
M third_party/subzero/src/IceInst.h
M third_party/subzero/src/IceTargetLowering.cpp
M third_party/subzero/src/IceTargetLoweringARM32.cpp
M third_party/subzero/src/IceTargetLoweringMIPS32.cpp
M third_party/subzero/src/IceTargetLoweringX86BaseImpl.h

https://swiftshader-review.googlesource.com/52531

ap...@google.com <ap...@google.com> #5Feb 10, 2021 05:47AM

Project: SwiftShader
Branch: master

commit 8d50b556a5decb8a256c196282e42060a806fd35
Author: Nicolas Capens <capn@google.com>
Date: Fri Feb 05 14:55:30 2021

Discern between load and store addresses

There were InstLoad::getSourceAddress() and InstStore::getAddr()
methods, which aren't very clear and consistently named. This change
replaces them with getLoadAddress() and getStoreAddress(), respectively.

This will also enable moving these methods to the Inst class to make
them available for SubVectorLoad and SubVectorStore intrinsics. While
these methods don't make sense for other instructions, note that
Inst::getSrc() already provides access to all operands and has to be
used with knowledge of the operand meaning and layout. So this only
provides a name to these operands, and it would stick out as a sore
thumb if used incorrectly.

Bug:

b/179497998

Change-Id: I86b1201b8a1c611682f4f91541bdb49e17ef71a8
Reviewed-on:

https://swiftshader-review.googlesource.com/c/SwiftShader/+/52530
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>

M src/Reactor/Optimizer.cpp
M third_party/subzero/src/IceASanInstrumentation.cpp
M third_party/subzero/src/IceInst.cpp
M third_party/subzero/src/IceInst.h
M third_party/subzero/src/IceTargetLoweringARM32.cpp
M third_party/subzero/src/IceTargetLoweringMIPS32.cpp
M third_party/subzero/src/IceTargetLoweringX86BaseImpl.h

https://swiftshader-review.googlesource.com/52530

ap...@google.com <ap...@google.com> #6Feb 10, 2021 05:47AM

Project: SwiftShader
Branch: master

commit 99bbb14b1b98a467521552003cd0f5604186cbd4
Author: Nicolas Capens <capn@google.com>
Date: Fri Feb 05 14:13:37 2021

Eliminate the InstIntrinsicCall Target parameter

It is no longer used now that profiling support at the Subzero level
is eliminated.

This change adjusts all of the uses of getSrc() on intrinsics to obtain
the correct operand, but does not yet make simplifications based on
having them align with load/store instructions.

Bug:

b/179497998

Change-Id: I93705eaa1b7626184f612ab3a9755048004e531f
Reviewed-on:

https://swiftshader-review.googlesource.com/c/SwiftShader/+/52529
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>

M src/Reactor/Optimizer.cpp
M src/Reactor/SubzeroReactor.cpp
M third_party/subzero/src/IceInst.h
M third_party/subzero/src/IceTargetLoweringX86BaseImpl.h

https://swiftshader-review.googlesource.com/52529

ap...@google.com <ap...@google.com> #7Feb 10, 2021 05:47AM

Project: SwiftShader
Branch: master

commit d4f27d7a6da638c53010ec875b07891c273fb9f6
Author: Nicolas Capens <capn@google.com>
Date: Fri Feb 05 14:09:01 2021

Eliminate Subzero profiling support

We're never used this functionality, and shouldn't have a need for it.
Profiling information can be collected at the Reactor level or using
a profiler like VTune.

This functionality was the only thing using the `Target` parameter of
`InstIntrinsicCall`, which get in the way for aligning the parameters of
load- and store-like intrinsics with regular `InstLoad` and `InstStore`.

Bug:

b/179497998

Change-Id: I5a0ad5ee8e0101f0879a97a1ea01e3efc5bebbe4
Reviewed-on:

https://swiftshader-review.googlesource.com/c/SwiftShader/+/52528
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>

M third_party/subzero/src/IceCfg.cpp
M third_party/subzero/src/IceCfg.h
M third_party/subzero/src/IceCfgNode.cpp
M third_party/subzero/src/IceGlobalContext.cpp
M third_party/subzero/src/IceGlobalContext.h

https://swiftshader-review.googlesource.com/52528

ni...@google.com <ni...@google.com> #8Feb 10, 2021 06:45AM

Accepted by ni...@google.com.

Remaining work:

Eliminate InstIntrinsic::addArg(), getArg(), and getNumArgs() methods. Be careful not to make InstCall instructions use getSrc() and getSrcSize(), which could be done by adding private or protected overloads of it.
Find an elegant way to assert that Inst::getLoadAddress(), getStoreAddress(), and getData() are only used on Load and Store instructions, and LoadSubVector and StoreSubVector intrinsics.

is...@google.com <is...@google.com> Jan 25, 2023 03:56PM

Status: New

Issue 179497998

Description

Issue summary

Comments

ni...@google.com <ni...@google.com> #2Feb 10, 2021 01:19AM

ap...@google.com <ap...@google.com> #3Feb 10, 2021 05:47AM

ap...@google.com <ap...@google.com> #4Feb 10, 2021 05:47AM

ap...@google.com <ap...@google.com> #5Feb 10, 2021 05:47AM

ap...@google.com <ap...@google.com> #6Feb 10, 2021 05:47AM

ap...@google.com <ap...@google.com> #7Feb 10, 2021 05:47AM

ni...@google.com <ni...@google.com> #8Feb 10, 2021 06:45AM

is...@google.com <is...@google.com> Jan 25, 2023 03:56PM

Add comment

Issue metadata