Skip to content

[SYCL] Added a new category for device binary properties - #3072

Merged
bader merged 3 commits into
intel:syclfrom
DenisBakhvalov:mix-sycl-esimd-rt-1
Feb 1, 2021
Merged

bader merged 3 commits into
intel:syclfrom
DenisBakhvalov:mix-sycl-esimd-rt-1

Conversation

@DenisBakhvalov

@DenisBakhvalov DenisBakhvalov commented Jan 21, 2021 •

Copy link
Copy Markdown
Contributor

This PR introduces a new category for general properties of device binary images. And it adds an API to retrieve properties from that common category.

Comment thread sycl/source/detail/pi.cpp Outdated
smaslov-intel
smaslov-intel previously approved these changes Jan 22, 2021

@smaslov-intel smaslov-intel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pi changes look good to me

bader
bader previously approved these changes Jan 22, 2021

@bader bader left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

llvm/* part looks good to me.

@bader
bader requested a review from kbobrovs January 23, 2021 09:50
kbobrovs
kbobrovs previously approved these changes Jan 26, 2021
Comment thread sycl/include/CL/sycl/detail/pi.hpp Outdated
@DenisBakhvalov
DenisBakhvalov dismissed stale reviews from kbobrovs, bader, and smaslov-intel via d0939fc January 27, 2021 18:07
romanovvlad
romanovvlad previously approved these changes Jan 27, 2021
@DenisBakhvalov

Copy link
Copy Markdown
Contributor Author

I think I have all the approvals here. @bader or @kbobrovs, please merge.

"SYCL/composite specialization constants";
static constexpr char SYCL_DEVICELIB_REQ_MASK[] = "SYCL/devicelib req mask";
static constexpr char SYCL_KERNEL_PARAM_OPT_INFO[] = "SYCL/kernel param opt";
static constexpr char SYCL_IS_ESIMD_IMAGE[] = "SYCL/is ESIMD image";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DenisBakhvalov, as we agreed, can you please change the category name for this to something like SYCL/misc or SYCL/binary image?

@kbobrovs

Copy link
Copy Markdown
Contributor

I think I have all the approvals here. @bader or @kbobrovs, please merge.

@DenisBakhvalov, please see comment above. If believe the category name change is still pending, right?

@DenisBakhvalov

Copy link
Copy Markdown
Contributor Author

I think I have all the approvals here. @bader or @kbobrovs, please merge.

@DenisBakhvalov, please see comment above. If believe the category name change is still pending, right?

@kbobrovs Yes, I'm working on it.

@DenisBakhvalov DenisBakhvalov changed the title [SYCL] Added a new device binary property for ESIMD images [SYCL] Added a new category for device binary properties Jan 30, 2021

@kbobrovs kbobrovs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@dendibakh

Copy link
Copy Markdown
Contributor

@bader @kbobrovs , please merge if no objections.

@bader

bader commented Feb 1, 2021

Copy link
Copy Markdown
Contributor

image

The approval from @smaslov-intel is required. There were changes in the code he owns since his latest review.

@smaslov-intel smaslov-intel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bader
bader merged commit 31edc2b into intel:sycl Feb 1, 2021
iclsrc pushed a commit that referenced this pull request Oct 11, 2026
…(#227627)

CodeGenPrepare::splitLargeGEPOffsets collects GEP candidates with large
constant offsets and rebases them to a shared common base, but only for
positive offsets (`ConstantOffset > 0`). Negative far-offset accesses
are left with independent `SUBXri` base materializations per access,
giving them different base registers and preventing the
`LoadStoreOptimizer` from pairing them into `LDP/STP`.

Relax the gate to `ConstantOffset != 0`, and extend
`AArch64TargetLowering::getPreferredLargeGEPBaseOffset` to handle
negative offsets using the same `HighPart = MinOffset & ~0xfff` rebase
as positive ones. For negative `MinOffset`, `HighPart` is the next lower
4096-aligned address, so all residuals (`Offset - HighPart`) are
non-negative and fit `LDR/STR`'s 12-bit unsigned scaled immediate. When
the residual also falls within `LDP/STP`'s 7-bit pairing range, `LSO`
pairs directly; otherwise `LSO`'s base-adjust (#223684) folds in an
extra `ADDXri` (or merges it into a preceding `SUBXri`/`ADDXri` when
adjacent), recovering the same instruction count as a direct rebase to
`MinOffset`.

This subsumes the MIR-level `shareBaseAddresses` pass (#223682) for the
negative-offset case, doing the equivalent work earlier at IR level. It
also benefits cases where LDP pairing isn't possible (non-adjacent
siblings) — the shared base alone saves instructions via CSE, which a
per-BB MIR-level pass can't do.

Example (from https://godbolt.org/z/sjo8eMW6j, 3 non-adjacent
negative-offset siblings, no LDP pairing):
; Before (upstream/main):
mov x8, #-4
mov x9, #-64
mov x10, #-1024
movk x8, #65531, lsl #16
movk x9, #65531, lsl #16
movk x10, #65531, lsl #16
ldr w8, x0, x8
ldr w9, x0, x9
ldr w10, x0, x10
; 9 instructions

; After (this patch):
sub x8, x0, #65, lsl #12   ; =266240
ldr w9, [x8, #4092]
ldr w10, [x8, #4032]
ldr w8, [x8, #3072]
; 4 instructions vs 9

Testing:
- Full AArch64 CodeGen test suite with assertions enabled (4301 tests, 0
regressions; 2 pre-existing upstream failures in
`arm64-codegen-prepare-extload.ll` and `and-sink.ll`, unrelated to this
change)
- `llvm-test-suite` (3535 passed, 0 failures, 0 errors)
- Bootstrap: building the entire LLVM+Clang with the patched clang
(assertions on, no crashes)

Note: `AArch64` is the only target that overrides
`shouldConsiderGEPOffsetSplit` and `getPreferredLargeGEPBaseOffset`, so
the gate change only affects AArch64 in practice.

Assisted-by: Claude (OpenCode)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants