(release 20.x non-upstream patch) [LV] Add initial support for vectorizing literal struct return values by MacDue · Pull Request #82 · arm/arm-toolchain

MacDue · 2025-02-10T14:07:08Z

This patch adds initial support for vectorizing literal struct return values. Currently, this is limited to the case where the struct is homogeneous (all elements have the same type) and not packed. The users of the call also must all be extractvalue instructions.

The intended use case for this is vectorizing intrinsics such as:

declare { float, float } @llvm.sincos.f32(float %x)

Mapping them to structure-returning library calls such as:

declare { <4 x float>, <4 x i32> } @Sleef_sincosf4_u10advsimd(<4 x float>)

Or their widened form (such as @llvm.sincos.v4f32 in this case).

Implementing this required two main changes:

Supporting widening extractvalue
Adding support for vectorized struct types in LV

This is mostly limited to parts of the cost model and scalarization

Since the supported use case is narrow, the required changes are relatively small.

Downstream issue: #87

This patch adds initial support for vectorizing literal struct return values. Currently, this is limited to the case where the struct is homogeneous (all elements have the same type) and not packed. The users of the call also must all be `extractvalue` instructions. The intended use case for this is vectorizing intrinsics such as: ``` declare { float, float } @llvm.sincos.f32(float %x) ``` Mapping them to structure-returning library calls such as: ``` declare { <4 x float>, <4 x i32> } @Sleef_sincosf4_u10advsimd(<4 x float>) ``` Or their widened form (such as `@llvm.sincos.v4f32` in this case). Implementing this required two main changes: 1. Supporting widening `extractvalue` 2. Adding support for vectorized struct types in LV * This is mostly limited to parts of the cost model and scalarization Since the supported use case is narrow, the required changes are relatively small.

MacDue · 2025-02-10T14:09:14Z

~~This PR is a copy of the LLVM PR llvm/llvm-project#109833, which was not possible to upstream for the release.~~

PR now merged upstream.

dcandler

Since this is a downstream change, it should follow the downstream patch policy: https://github.com/arm/arm-toolchain/blob/arm-software/CONTRIBUTING.md#downstream-patch-policy

If it's a critical change that didn't make the upstream release, then please make sure it's tracked appropriately.

MacDue · 2025-02-11T11:50:08Z

Hey @dcandler, I've gone over the requirements and attempted to address them for this PR. The main one that's unclear to me is "source change should be annotated with a comment", which seems slightly impractical for non-trivial changes
(e.g. this change spans multiple files and functions).

The pull request description or comment must contain a justification of why the patch isn't being made upstream. This should include links to any previous attempts to upstream.

PRs include links to LLVM patches, the main release for the patch(es) not being upstream is long review times.

The commit message for a new downstream patch must include Downstream issue:# where # is an issue that contains the reason for the downstream patch.

I've created tracking issue #87.

The source change should be annotated with a comment including the #, if there are multiple lines changed a single comment for the block can be used.

I'm not sure how to go about this one. It seems impractical for non-trivial changes.

The commit message that removes an existing downstream patch, such as when the upstream equivalent lands, must include Removes downstream issue:#.

N/A (yet)

New files in the arm-toolchain repository must include the LLVM license, with the standard header comment and SPDX identifier.

N/A

New files added to an external project like picolibc or newlib must follow the licensing and copyright of the external project.

N/A

Any patch that causes the Arm Toolchain to deviate from upstream behavior, such as a new command-line option, must have user documentation. See Downstream Feature Documentation for the location and required contents.

Only #84 should noticeably change behaviour (as sincos calls become vectorizable with -fveclib=ArmPL/SLEEF)

MacDue · 2025-02-12T11:34:38Z

cc @david-arm

dcandler · 2025-02-12T12:22:59Z

Thanks - this is the first time attempting to apply the policy, so it's also a bit of a test of the policy itself.

The commit message for a new downstream patch must include Downstream issue:# where # is an issue that contains the reason for the downstream patch.

I think the text in the commit message might need to be exactly Downstream issue:#87 since the intention is to have scripting be able to read the issue number (e.g. in a pre-merge check) and I'm not sure how strict the scripts will be with keeping the formatting consistent.

The source change should be annotated with a comment including the #, if there are multiple lines changed a single comment for the block can be used.

I can see it as being a bit of a burden here, but it's also meant to help with the automated scanning of the sources to ensure everything is tracked. Again though, the scripts and infrastructure for those checks is not in place yet - so if we did relax that requirement it might still need adding back in later.
However the other motivation for this was to make it easier to see where/why something was changed when resolving a merge conflict, which any downstream change has the potential to create. But on a release branch, I'd expect a conflict less often.

MacDue · 2025-02-12T13:52:33Z

I think the text in the commit message might need to be exactly Downstream issue:#87 since the intention is to have scripting be able to read the issue number (e.g. in a pre-merge check) and I'm not sure how strict the scripts will be with keeping the formatting consistent.

Just checking: Are PRs to this repo squash merged (like upstream LLVM) -- so only the PR description needs updating to update the commit message?

I can see it as being a bit of a burden here, but it's also meant to help with the automated scanning of the sources to ensure everything is tracked.

What do you suggest we do for these PRs?

dcandler · 2025-02-12T14:51:48Z

Yes, squashing is enabled (and currently the default) so you can use the updated description when merging.

For the annotations, I think we should stick to the policy as written since that's what was agreed; if it's not followed then it defeats the purpose of having the policy in the first place.

MacDue · 2025-02-12T16:22:32Z

I've added the annotations for this PR. Since there are no examples in the guide, I just made up a format for them.

dcandler

Formatting of the annotations looks fine to me - we can probably expand the policy with more examples/guidelines once we've gotten to see how it works out in practice (and any automated checking is implemented, if anything stricter is needed).

This reverts commit 8065001.

MacDue · 2025-02-17T17:11:05Z

The corresponding upstream PR has now been merged (llvm/llvm-project#109833), so I've removed the comments as this is now a simple cherry-pick.

The other PRs are still downstream.

dcandler · 2025-02-18T10:13:38Z

A cherry pick to our downstream release branch is still considered a downstream change to which the policy would apply, since it causes our branch to diverge from the upstream version. To avoid making it a downstream change, it would need to be cherry-picked to the upstream release branch, where it could be automerged to ours.

This reverts commit 199a9ec.

MacDue · 2025-02-18T10:16:30Z

Alright, I've added the comments back 👍

MacDue · 2025-02-24T14:22:54Z

@dcandler Is there anything blocking the merge of this PR? I don't have write access to this repository so I need someone to merge this for me.

dcandler · 2025-02-24T14:46:24Z

Looks fine to me - I'll go ahead and merge if you like. I saw the next in the series is out of draft so I'll check that next.

MacDue · 2025-02-24T14:48:32Z

Looks fine to me - I'll go ahead and merge if you like. I saw the next in the series is out of draft so I'll check that next.

Cool, thank you!

Summary This builds on the accelerator plugin protocol (llvm/llvm-project#201489) by letting a plugin ask the client to create and connect a second target — the mechanism a real backend (e.g. a GPU debug stub) uses to surface the accelerator alongside the CPU process being debugged. ### What this adds **Protocol** — a new `AcceleratorConnectionInfo` describing how the client should bring up the accelerator target. **Client** — when an `AcceleratorActions` carries `connect_info`, `ProcessGDBRemote` creates a new (empty) target, reverse-connects it to the GDB server the plugin points. **Mock server (for testing)** — to exercise this end to end, the mock accelerator plugin stands up an in-process GDB server backed by a minimal fake process: `ProcessMockAccelerator`, `ThreadMockAccelerator` and `RegisterContextMockAccelerator`. ### Scope Intentionally minimal: the goal is to create and show the second (accelerator) target. There is no register/memory modeling beyond what a connection requires. Next set of PRs will build up on this. ### Testing Demo of how this looks with this PR. (explicitly asking the native process to stop at every breakpoint for testability) ``` satyajanga@devgpu011:toolchain $ ./bin/lldb (lldb) file /home/satyajanga/llvm-sand/build/Debug/fbcode-x86_64/toolchain/lldb-test-build.noindex/accelerator/mock/TestMockAcceleratorActions/a.out Current executable set to '/home/satyajanga/llvm-sand/build/Debug/fbcode-x86_64/toolchain/lldb-test-build.noindex/accelerator/mock/TestMockAcceleratorActions/a.out' (x86_64). (lldb) log enable -f /tmp/packets.log gdb-remote packets (lldb) r Process 1430531 launched: '/home/satyajanga/llvm-sand/build/Debug/fbcode-x86_64/toolchain/lldb-test-build.noindex/accelerator/mock/TestMockAcceleratorActions/a.out' (x86_64) Process 1430531 stopped (lldb) target list Current targets: * target #0: /home/satyajanga/llvm-sand/build/Debug/fbcode-x86_64/toolchain/lldb-test-build.noindex/accelerator/mock/TestMockAcceleratorActions/a.out ( arch=x86_64-unknown-linux-gnu, platform=host, pid=1430531, state=stopped ) (lldb) c Process 1430531 resuming Process 1 stopped * thread #1, name = 'Mock Accelerator Thread', stop reason = trace frame #0: 0x0000000000001004 error: memory read failed for 0x1000 Target 0: (a.out) stopped. (lldb) target list Current targets: target #0: /home/satyajanga/llvm-sand/build/Debug/fbcode-x86_64/toolchain/lldb-test-build.noindex/accelerator/mock/TestMockAcceleratorActions/a.out ( arch=x86_64-unknown-linux-gnu, platform=host, pid=1430531, state=stopped ) * target #1: <none> ( arch=x86_64-unknown-linux-gnu, platform=host, pid=1, state=stopped ) (lldb) c Process 1 resuming (lldb) target select 0 Current targets: * target #0: /home/satyajanga/llvm-sand/build/Debug/fbcode-x86_64/toolchain/lldb-test-build.noindex/accelerator/mock/TestMockAcceleratorActions/a.out ( arch=x86_64-unknown-linux-gnu, platform=host, pid=1430531, state=stopped ) target #1: <none> ( arch=x86_64-unknown-linux-gnu, platform=host, pid=1, state=running ) (lldb) c Process 1430531 resuming Process 1430531 stopped (lldb) target list Current targets: * target #0: /home/satyajanga/llvm-sand/build/Debug/fbcode-x86_64/toolchain/lldb-test-build.noindex/accelerator/mock/TestMockAcceleratorActions/a.out ( arch=x86_64-unknown-linux-gnu, platform=host, pid=1430531, state=stopped ) target #1: <none> ( arch=x86_64-unknown-linux-gnu, platform=host, pid=1, state=running ) (lldb) a.out │ internal accelerator-plugin (mock) breakpoint(-4). ``` Packet log, you can notice the actions set with breakpoint info and later responding with connection info. and you can also see the 5 GPRs ``` lldb < 104> send packet: $qSupported:xmlRegisters=i386,arm,mips,arc;multiprocess+;fork-events+;vfork-events+;swbreak+;hwbreak+#cd lldb < 299> read packet: $PacketSize=131072;QStartNoAckMode+;qEcho+;native-signals+;QThreadSuffixSupported+;QListThreadsInStopReply+;qXfer:features:read+;QNonStop+;jMultiBreakpoint+;QPassSignals+;qXfer:auxv:read+;qXfer:libraries-svr4:read+;qXfer:siginfo:read+;accelerator-plugins+;multiprocess+;fork-events+;vfork-events+#86 .. lldb < 32> send packet: $jAcceleratorPluginInitialize#50 lldb < 238> read packet: $[{"breakpoints":[{"by_address":null,"by_name":{"function_name":"mock_gpu_accelerator_initialize","shlib":null}],"identifier":1,"symbol_names":["mock_gpu_accelerator_compute"]}]],"identifier":1,"plugin_name":"mock","session_name":""}]]#2f .. intern-state < 314> send packet: $jAcceleratorPluginBreakpointHit:{"breakpoint":{"by_address":null,"by_name":{"function_name":"mock_gpu_accelerator_initialize","shlib":null}],"identifier":1,"symbol_names":["mock_gpu_accelerator_compute"]}],"plugin_name":"mock","symbol_values":[{"name":"mock_gpu_accelerator_compute","value":93824992235840}]]}]#e7 intern-state < 487> read packet: ${"actions":{"breakpoints":[{"by_address":null,"by_name":{"function_name":"mock_gpu_accelerator_finish","shlib":"a.out"}],"identifier":3,"symbol_names":[]}],{"by_address":{"load_address":93824992235840}],"by_name":null,"identifier":2,"symbol_names":[]}],{"by_address":null,"by_name":{"function_name":"mock_gpu_accelerator_connect","shlib":null}],"identifier":4,"symbol_names":[]}]],"identifier":2,"plugin_name":"mock","session_name":""}],"auto_resume_native":false,"disable_bp":true}]#7d .. .. intern-state < 218> send packet: $jAcceleratorPluginBreakpointHit:{"breakpoint":{"by_address":null,"by_name":{"function_name":"mock_gpu_accelerator_connect","shlib":null}],"identifier":4,"symbol_names":[]}],"plugin_name":"mock","symbol_values":[]}]#d1 intern-state < 268> read packet: ${"actions":{"breakpoints":[],"connect_info":{"connect_url":"connect://localhost:32893","exe_path":null,"synchronous":true,"triple":null}],"identifier":4,"plugin_name":"mock","session_name":"Mock Accelerator Session"}],"auto_resume_native":false,"disable_bp":true}]#8e intern-state < 1> send packet: + intern-state history[1] tid=0x15d41f < 1> send packet: + intern-state < 19> send packet: $QStartNoAckMode#b0 intern-state < 1> read packet: + intern-state < 6> read packet: $OK#9a intern-state < 1> send packet: + intern-state < 104> send packet: $qSupported:xmlRegisters=i386,arm,mips,arc;multiprocess+;fork-events+;vfork-events+;swbreak+;hwbreak+#cd intern-state < 159> read packet: $PacketSize=131072;QStartNoAckMode+;qEcho+;native-signals+;QThreadSuffixSupported+;QListThreadsInStopReply+;qXfer:features:read+;QNonStop+;jMultiBreakpoint+#f3 intern-state < 26> send packet: $QThreadSuffixSupported#e4 intern-state < 6> read packet: $OK#9a intern-state < 27> send packet: $QListThreadsInStopReply#21 intern-state < 6> read packet: $OK#9a intern-state < 13> send packet: $qHostInfo#9b intern-state < 364> read packet: $triple:7838365f36342d2d6c696e75782d676e75;ptrsize:8;watchpoint_exceptions_received:after;endian:little;os_version:6.13.2;os_build:362e31332e322d305f66626b355f 68617264656e65645f7263325f305f67313733613962316463613431;os_kernel:233120534d5020576564204a756c2032332030393a32343a3330205044542032303235;hostname:6465766770753031312e656167322e66616365626f6f6b2e6 36f6d;#c4 intern-state < 10> send packet: $vCont?#49 intern-state < 19> read packet: $vCont;c;C;s;S;t#11 intern-state < 27> send packet: $qVAttachOrWaitSupported#38 intern-state < 6> read packet: $OK#9a intern-state < 23> send packet: $QEnableErrorStrings#8c intern-state < 6> read packet: $OK#9a intern-state < 16> send packet: $qProcessInfo#dc intern-state < 7> read packet: $E01#a6 intern-state < 6> send packet: $qC#b4 intern-state < 7> read packet: $QC1#c5 intern-state < 5> send packet: $?#3f intern-state < 291> read packet: $T00thread:1;name:Mock Accelerator Thread;threads:1;thread-pcs:0000000000001004;00:0010000000000000;01:0110000000000000;02:0210000000000000;03:0310000000000000 ;04:0410000000000000;05:0510000000000000;reason:trace;description:6d6f636b20616363656c657261746f72207468726561642073746f70706564;#83 intern-state < 16> send packet: $qProcessInfo#dc intern-state < 7> read packet: $E01#a6 intern-state < 16> send packet: $qProcessInfo#dc intern-state < 7> read packet: $E01#a6 intern-state < 43> send packet: $qXfer:features:read:target.xml:0,131071#78 intern-state < 889> read packet: $l<?xml version="1.0"?> <target version="1.0"> <architecture>x86_64</architecture> <feature> <reg name="r0" bitsize="64" regnum="0" offset="0" encoding="uint" format="hex" group="General Purpose Registers" /> <reg name="r1" bitsize="64" regnum="1" offset="8" encoding="uint" format="hex" group="General Purpose Registers" /> <reg name="sp" bitsize="64" regnum="2" offset="16" encoding="uint" format="hex" group="General Purpose Registers" generic="sp" /> <reg name="fp" bitsize="64" regnum="3" offset="24" encoding="uint" format="hex" group="General Purpose Registers" generic="fp" /> <reg name="pc" bitsize="64" regnum="4" offset="32" encoding="uint" format="hex" group="General Purpose Registers" generic="pc" /> <reg name="flags" bitsize="64" regnum="5" offset="40" encoding="uint" format="hex" group="General Purpose Registers" /> </feature> </target> #82 intern-state < 19> send packet: $p0;thread:0001;#89 intern-state < 20> read packet: $0010000000000000#01 intern-state < 16> send packet: $qProcessInfo#dc intern-state < 7> read packet: $E01#a6 intern-state < 19> send packet: $p0;thread:0001;#89 intern-state < 20> read packet: $0010000000000000#01 intern-state < 16> send packet: $qProcessInfo#dc intern-state < 7> read packet: $E01#a6 intern-state < 16> send packet: $qProcessInfo#dc intern-state < 7> read packet: $E01#a6 intern-state < 16> send packet: $qProcessInfo#dc intern-state < 7> read packet: $E01#a6 intern-state < 26> send packet: $qStructuredDataPlugins#02 intern-state < 6> read packet: $[]#b8 intern-state < 26> send packet: $qMemoryRegionInfo:1004#d9 intern-state < 41> read packet: $error:6e6f7420696d706c656d656e746564;#f7 intern-state < 16> send packet: $jThreadsInfo#c1 intern-state < 198> read packet: $[{"description":"mock accelerator thread stopped","name":"Mock Accelerator Thread","reason":"trace","registers":{"2":"0210000000000000","3":"0310000000000000" ,"4":"0410000000000000"}],"tid":1}]]#4a ```

MacDue changed the title ~~(release 20.x cherry-pick) [LV] Add initial support for vectorizing literal struct return values~~ (release 20.x non-upstream patch) [LV] Add initial support for vectorizing literal struct return values Feb 10, 2025

kiranchandramohan requested review from DaftanoPro and pawosm-arm February 10, 2025 14:15

pawosm-arm previously approved these changes Feb 10, 2025

View reviewed changes

MacDue mentioned this pull request Feb 10, 2025

(release 20.x non-upstream patch) [LV] Teach the vectorizer to cost and vectorize llvm.sincos intrinsics #84

Merged

dcandler requested changes Feb 11, 2025

View reviewed changes

MacDue mentioned this pull request Feb 11, 2025

[Downstream Change] sincos() vectorization #87

Open

4 tasks

Add a load of comments

8065001

dcandler approved these changes Feb 13, 2025

View reviewed changes

Revert "Add a load of comments"

199a9ec

This reverts commit 8065001.

MacDue changed the title ~~(release 20.x non-upstream patch) [LV] Add initial support for vectorizing literal struct return values~~ (release 20.x cherry-pick) [LV] Add initial support for vectorizing literal struct return values Feb 17, 2025

Revert "Revert "Add a load of comments""

545a1bd

This reverts commit 199a9ec.

MacDue changed the title ~~(release 20.x cherry-pick) [LV] Add initial support for vectorizing literal struct return values~~ (release 20.x non-upstream patch) [LV] Add initial support for vectorizing literal struct return values Feb 18, 2025

MacDue dismissed pawosm-arm’s stale review via 545a1bd February 19, 2025 11:09

dcandler merged commit c742ca2 into arm:release/arm-software/20.x Feb 24, 2025

MacDue deleted the release_20_struct_loop_vec branch February 24, 2025 14:58

This was referenced Feb 28, 2025

rebase perf patches #141

Closed

[ATfE] Cherry-pick libcxx change in wchar inclusion guarding into 20.x #153

Merged

Uh oh!

Conversation

MacDue commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MacDue commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcandler left a comment

Choose a reason for hiding this comment

Uh oh!

MacDue commented Feb 11, 2025

Uh oh!

MacDue commented Feb 12, 2025

Uh oh!

dcandler commented Feb 12, 2025

Uh oh!

MacDue commented Feb 12, 2025

Uh oh!

dcandler commented Feb 12, 2025

Uh oh!

MacDue commented Feb 12, 2025

Uh oh!

dcandler left a comment

Choose a reason for hiding this comment

Uh oh!

MacDue commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcandler commented Feb 18, 2025

Uh oh!

MacDue commented Feb 18, 2025

Uh oh!

MacDue commented Feb 24, 2025

Uh oh!

dcandler commented Feb 24, 2025

Uh oh!

MacDue commented Feb 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MacDue commented Feb 10, 2025 •

edited

Loading

MacDue commented Feb 10, 2025 •

edited

Loading

MacDue commented Feb 17, 2025 •

edited

Loading