From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x62d.google.com (mail-ej1-x62d.google.com [IPv6:2a00:1450:4864:20::62d]) by sourceware.org (Postfix) with ESMTPS id 76AFB3858CDA for ; Fri, 12 Aug 2022 11:14:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 76AFB3858CDA Received: by mail-ej1-x62d.google.com with SMTP id uj29so1523802ejc.0 for ; Fri, 12 Aug 2022 04:14:35 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc; bh=QF4HzgeYpRJpKV2r/GdgwBeIqoKDn0j/0ito01uwNdY=; b=vWzXp2t4CEiOd1ohniPO66AWQ33DHP3UyEwvVAVzT9NDVmMxXhS01GR0l04ixSCG4F 8jiw7omdYRho2sd9H++aF23bkpegctMzEcuD2oAIq+IWI2/8O+S/vKzmOdpvld107ejb 35hjwYIebCDKKTOVQJLUq5MPzwS6sr+QueGaezBbG3aUIOa/ouYMUpiBZ6lIKzve/joi S2dhcwniKYXlDORhnq3Wbwd8JDIgU69I5eGCPRkGndV+jrVaeBtGXsesgkkPQYFIsOxX 5qXKtKg3f8impqPDdDnakJLT5G6GZJSM1BKcvUy8kQt9QDIViy4UFmbQdFrZPy+Eb2cu 0l6g== X-Gm-Message-State: ACgBeo1VNZtq9QFVObtwUSqBtGcmptCEUNQBiOx5hVvWwsLKJuI4wwJk cj8DMkKN7cMtVKxOVx7c99Rt0ikphTqH3M4NeBU= X-Google-Smtp-Source: AA6agR5bHEDVlv7EWSPygo0hbhQJg+HoaCk1lCdLqsyjyVDuOo/Ej3Bi7HGK6Z8nSefuGhGwRRvuBdJdJ8oRHyxGv+M= X-Received: by 2002:a17:907:3e29:b0:730:9d82:5113 with SMTP id hp41-20020a1709073e2900b007309d825113mr2429016ejc.29.1660302873557; Fri, 12 Aug 2022 04:14:33 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Richard Biener Date: Fri, 12 Aug 2022 13:14:20 +0200 Message-ID: Subject: Re: [PATCH] vect: Don't allow vect_emulated_vector_p type in vectorizable_call [PR106322] To: "Kewen.Lin" Cc: GCC Patches , Richard Sandiford , Andrew Pinski , Uros Bizjak Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 12 Aug 2022 11:14:38 -0000 On Fri, Aug 12, 2022 at 11:41 AM Kewen.Lin wrote: > > Hi, > > As PR106322 shows, in some cases for some vector type whose > TYPE_MODE is a scalar integral mode instead of a vector mode, > it's possible to obtain wrong target support information when > querying with the scalar integral mode. For example, for the > test case in PR106322, on ppc64 32bit vectorizer gets vector > type "vector(2) short unsigned int" for scalar type "short > unsigned int", its mode is SImode instead of V2HImode. The > target support querying checks umul_highpart optab with SImode > and considers it's supported, then vectorizer further generates > .MULH IFN call for that vector type. Unfortunately it's wrong > to use SImode support for that vector type multiply highpart > here. > > This patch is to teach vectorizable_call analysis not to allow > vect_emulated_vector_p type for both vectype_in and vectype_out > as Richi suggested. > > Bootstrapped and regtested on x86_64-redhat-linux, > aarch64-linux-gnu and powerpc64{,le}-linux-gnu. > > Is it ok for trunk? OK for trunk. > If it's ok, I guess we want this to be > backported? Yes, but you just missed the RC for 12.2 so please wait until after GCC 12.2 is released and the branch is open again. The testcase looks mightly complicated so fallout there might be well possible as well ;) I suppose it wasn't possible to craft a simple C testcase after the analysis? Richard. > > BR, > Kewen > ----- > PR tree-optimization/106322 > > gcc/ChangeLog: > > * tree-vect-stmts.cc (vectorizable_call): Don't allow > vect_emulated_vector_p type for both vectype_in and vectype_out. > > gcc/testsuite/ChangeLog: > > * g++.target/i386/pr106322.C: New test. > * g++.target/powerpc/pr106322.C: New test. > --- > gcc/testsuite/g++.target/i386/pr106322.C | 196 ++++++++++++++++++++ > gcc/testsuite/g++.target/powerpc/pr106322.C | 195 +++++++++++++++++++ > gcc/tree-vect-stmts.cc | 8 + > 3 files changed, 399 insertions(+) > create mode 100644 gcc/testsuite/g++.target/i386/pr106322.C > create mode 100644 gcc/testsuite/g++.target/powerpc/pr106322.C > > diff --git a/gcc/testsuite/g++.target/i386/pr106322.C b/gcc/testsuite/g++.target/i386/pr106322.C > new file mode 100644 > index 00000000000..3cd8d6bf225 > --- /dev/null > +++ b/gcc/testsuite/g++.target/i386/pr106322.C > @@ -0,0 +1,196 @@ > +/* { dg-do run } */ > +/* { dg-require-effective-target ia32 } */ > +/* { dg-require-effective-target c++11 } */ > +/* { dg-options "-O2 -mtune=generic -march=i686" } */ > + > +/* As PR106322, verify this can execute well (not abort). */ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +__attribute__((noipa)) > +bool BytesEqual(const void *bytes1, const void *bytes2, const size_t size) { > + return memcmp(bytes1, bytes2, size) == 0; > +} > + > +#define HWY_ALIGNMENT 64 > +constexpr size_t kAlignment = HWY_ALIGNMENT; > +constexpr size_t kAlias = kAlignment * 4; > + > +namespace hwy { > +namespace N_EMU128 { > +template struct Vec128 { > + T raw[16 / sizeof(T)] = {}; > +}; > +} // namespace N_EMU128 > +} // namespace hwy > + > +template > +static void Store(const hwy::N_EMU128::Vec128 v, > + T *__restrict__ aligned) { > + __builtin_memcpy(aligned, v.raw, sizeof(T) * N); > +} > + > +template > +static hwy::N_EMU128::Vec128 Load(const T *__restrict__ aligned) { > + hwy::N_EMU128::Vec128 v; > + __builtin_memcpy(v.raw, aligned, sizeof(T) * N); > + return v; > +} > + > +template > +static hwy::N_EMU128::Vec128 > +MulHigh(hwy::N_EMU128::Vec128 a, > + const hwy::N_EMU128::Vec128 b) { > + for (size_t i = 0; i < N; ++i) { > + // Cast to uint32_t first to prevent overflow. Otherwise the result of > + // uint16_t * uint16_t is in "int" which may overflow. In practice the > + // result is the same but this way it is also defined. > + a.raw[i] = static_cast( > + (static_cast(a.raw[i]) * static_cast(b.raw[i])) >> > + 16); > + } > + return a; > +} > + > +#define HWY_ASSERT(condition) assert((condition)) > +#define HWY_ASSUME_ALIGNED(ptr, align) __builtin_assume_aligned((ptr), (align)) > + > +#pragma pack(push, 1) > +struct AllocationHeader { > + void *allocated; > + size_t payload_size; > +}; > +#pragma pack(pop) > + > +static void FreeAlignedBytes(const void *aligned_pointer) { > + HWY_ASSERT(aligned_pointer != nullptr); > + if (aligned_pointer == nullptr) > + return; > + > + const uintptr_t payload = reinterpret_cast(aligned_pointer); > + HWY_ASSERT(payload % kAlignment == 0); > + const AllocationHeader *header = > + reinterpret_cast(payload) - 1; > + > + free(header->allocated); > +} > + > +class AlignedFreer { > +public: > + template void operator()(T *aligned_pointer) const { > + FreeAlignedBytes(aligned_pointer); > + } > +}; > + > +template > +using AlignedFreeUniquePtr = std::unique_ptr; > + > +static inline constexpr size_t ShiftCount(size_t n) { > + return (n <= 1) ? 0 : 1 + ShiftCount(n / 2); > +} > + > +namespace { > +static size_t NextAlignedOffset() { > + static std::atomic next{0}; > + constexpr uint32_t kGroups = kAlias / kAlignment; > + const uint32_t group = next.fetch_add(1, std::memory_order_relaxed) % kGroups; > + const size_t offset = kAlignment * group; > + HWY_ASSERT((offset % kAlignment == 0) && offset <= kAlias); > + return offset; > +} > +} // namespace > + > +static void *AllocateAlignedBytes(const size_t payload_size) { > + HWY_ASSERT(payload_size != 0); // likely a bug in caller > + if (payload_size >= std::numeric_limits::max() / 2) { > + HWY_ASSERT(false && "payload_size too large"); > + return nullptr; > + } > + > + size_t offset = NextAlignedOffset(); > + > + // What: | misalign | unused | AllocationHeader |payload > + // Size: |<= kAlias | offset |payload_size > + // ^allocated.^aligned.^header............^payload > + // The header must immediately precede payload, which must remain aligned. > + // To avoid wasting space, the header resides at the end of `unused`, > + // which therefore cannot be empty (offset == 0). > + if (offset == 0) { > + offset = kAlignment; // = RoundUpTo(sizeof(AllocationHeader), kAlignment) > + static_assert(sizeof(AllocationHeader) <= kAlignment, "Else: round up"); > + } > + > + const size_t allocated_size = kAlias + offset + payload_size; > + void *allocated = malloc(allocated_size); > + HWY_ASSERT(allocated != nullptr); > + if (allocated == nullptr) > + return nullptr; > + // Always round up even if already aligned - we already asked for kAlias > + // extra bytes and there's no way to give them back. > + uintptr_t aligned = reinterpret_cast(allocated) + kAlias; > + static_assert((kAlias & (kAlias - 1)) == 0, "kAlias must be a power of 2"); > + static_assert(kAlias >= kAlignment, "Cannot align to more than kAlias"); > + aligned &= ~(kAlias - 1); > + > + const uintptr_t payload = aligned + offset; // still aligned > + > + // Stash `allocated` and payload_size inside header for FreeAlignedBytes(). > + // The allocated_size can be reconstructed from the payload_size. > + AllocationHeader *header = reinterpret_cast(payload) - 1; > + header->allocated = allocated; > + header->payload_size = payload_size; > + > + return HWY_ASSUME_ALIGNED(reinterpret_cast(payload), kAlignment); > +}gcc-12.1.1+git287.tar.xz > + > +template static T *AllocateAlignedItems(size_t items) { > + constexpr size_t size = sizeof(T); > + > + constexpr bool is_pow2 = (size & (size - 1)) == 0; > + constexpr size_t bits = ShiftCount(size); > + static_assert(!is_pow2 || (1ull << bits) == size, "ShiftCount is incorrect"); > + > + const size_t bytes = is_pow2 ? items << bits : items * size; > + const size_t check = is_pow2 ? bytes >> bits : bytes / size; > + if (check != items) { > + return nullptr; // overflowed > + } > + return static_cast(AllocateAlignedBytes(bytes)); > +} > + > +template > +static AlignedFreeUniquePtr AllocateAligned(const size_t items) { > + return AlignedFreeUniquePtr(AllocateAlignedItems(items), > + AlignedFreer()); > +} > + > +int main() { > + AlignedFreeUniquePtr in_lanes = AllocateAligned(2); > + uint16_t expected_lanes[2]; > + in_lanes[0] = 65535; > + in_lanes[1] = 32767; > + expected_lanes[0] = 65534; > + expected_lanes[1] = 16383; > + hwy::N_EMU128::Vec128 v = Load(in_lanes.get()); > + hwy::N_EMU128::Vec128 actual = MulHigh(v, v); > + { > + auto actual_lanes = AllocateAligned(2); > + Store(actual, actual_lanes.get()); > + const uint8_t *expected_array = > + reinterpret_cast(expected_lanes); > + const uint8_t *actual_array = > + reinterpret_cast(actual_lanes.get()); > + for (size_t i = 0; i < 2; ++i) { > + const uint8_t *expected_ptr = expected_array + i * 2; > + const uint8_t *actual_ptr = actual_array + i * 2; > + if (!BytesEqual(expected_ptr, actual_ptr, 2)) { > + abort(); > + } > + } > + } > +} > diff --git a/gcc/testsuite/g++.target/powerpc/pr106322.C b/gcc/testsuite/g++.target/powerpc/pr106322.C > new file mode 100644 > index 00000000000..1de6e5e37e5 > --- /dev/null > +++ b/gcc/testsuite/g++.target/powerpc/pr106322.C > @@ -0,0 +1,195 @@ > +/* { dg-do run } */ > +/* { dg-require-effective-target c++11 } */ > +/* { dg-options "-O2 -mdejagnu-cpu=power4" } */ > + > +/* As PR106322, verify this can execute well (not abort). */ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +__attribute__((noipa)) > +bool BytesEqual(const void *bytes1, const void *bytes2, const size_t size) { > + return memcmp(bytes1, bytes2, size) == 0; > +} > + > +#define HWY_ALIGNMENT 64 > +constexpr size_t kAlignment = HWY_ALIGNMENT; > +constexpr size_t kAlias = kAlignment * 4; > + > +namespace hwy { > +namespace N_EMU128 { > +template struct Vec128 { > + T raw[16 / sizeof(T)] = {}; > +}; > +} // namespace N_EMU128 > +} // namespace hwy > + > +template > +static void Store(const hwy::N_EMU128::Vec128 v, > + T *__restrict__ aligned) { > + __builtin_memcpy(aligned, v.raw, sizeof(T) * N); > +} > + > +template > +static hwy::N_EMU128::Vec128 Load(const T *__restrict__ aligned) { > + hwy::N_EMU128::Vec128 v; > + __builtin_memcpy(v.raw, aligned, sizeof(T) * N); > + return v; > +} > + > +template > +static hwy::N_EMU128::Vec128 > +MulHigh(hwy::N_EMU128::Vec128 a, > + const hwy::N_EMU128::Vec128 b) { > + for (size_t i = 0; i < N; ++i) { > + // Cast to uint32_t first to prevent overflow. Otherwise the result of > + // uint16_t * uint16_t is in "int" which may overflow. In practice the > + // result is the same but this way it is also defined. > + a.raw[i] = static_cast( > + (static_cast(a.raw[i]) * static_cast(b.raw[i])) >> > + 16); > + } > + return a; > +} > + > +#define HWY_ASSERT(condition) assert((condition)) > +#define HWY_ASSUME_ALIGNED(ptr, align) __builtin_assume_aligned((ptr), (align)) > + > +#pragma pack(push, 1) > +struct AllocationHeader { > + void *allocated; > + size_t payload_size; > +}; > +#pragma pack(pop) > + > +static void FreeAlignedBytes(const void *aligned_pointer) { > + HWY_ASSERT(aligned_pointer != nullptr); > + if (aligned_pointer == nullptr) > + return; > + > + const uintptr_t payload = reinterpret_cast(aligned_pointer); > + HWY_ASSERT(payload % kAlignment == 0); > + const AllocationHeader *header = > + reinterpret_cast(payload) - 1; > + > + free(header->allocated); > +} > + > +class AlignedFreer { > +public: > + template void operator()(T *aligned_pointer) const { > + FreeAlignedBytes(aligned_pointer); > + } > +}; > + > +template > +using AlignedFreeUniquePtr = std::unique_ptr; > + > +static inline constexpr size_t ShiftCount(size_t n) { > + return (n <= 1) ? 0 : 1 + ShiftCount(n / 2); > +} > + > +namespace { > +static size_t NextAlignedOffset() { > + static std::atomic next{0}; > + constexpr uint32_t kGroups = kAlias / kAlignment; > + const uint32_t group = next.fetch_add(1, std::memory_order_relaxed) % kGroups; > + const size_t offset = kAlignment * group; > + HWY_ASSERT((offset % kAlignment == 0) && offset <= kAlias); > + return offset; > +} > +} // namespace > + > +static void *AllocateAlignedBytes(const size_t payload_size) { > + HWY_ASSERT(payload_size != 0); // likely a bug in caller > + if (payload_size >= std::numeric_limits::max() / 2) { > + HWY_ASSERT(false && "payload_size too large"); > + return nullptr; > + } > + > + size_t offset = NextAlignedOffset(); > + > + // What: | misalign | unused | AllocationHeader |payload > + // Size: |<= kAlias | offset |payload_size > + // ^allocated.^aligned.^header............^payload > + // The header must immediately precede payload, which must remain aligned. > + // To avoid wasting space, the header resides at the end of `unused`, > + // which therefore cannot be empty (offset == 0). > + if (offset == 0) { > + offset = kAlignment; // = RoundUpTo(sizeof(AllocationHeader), kAlignment) > + static_assert(sizeof(AllocationHeader) <= kAlignment, "Else: round up"); > + } > + > + const size_t allocated_size = kAlias + offset + payload_size; > + void *allocated = malloc(allocated_size); > + HWY_ASSERT(allocated != nullptr); > + if (allocated == nullptr) > + return nullptr; > + // Always round up even if already aligned - we already asked for kAlias > + // extra bytes and there's no way to give them back. > + uintptr_t aligned = reinterpret_cast(allocated) + kAlias; > + static_assert((kAlias & (kAlias - 1)) == 0, "kAlias must be a power of 2"); > + static_assert(kAlias >= kAlignment, "Cannot align to more than kAlias"); > + aligned &= ~(kAlias - 1); > + > + const uintptr_t payload = aligned + offset; // still aligned > + > + // Stash `allocated` and payload_size inside header for FreeAlignedBytes(). > + // The allocated_size can be reconstructed from the payload_size. > + AllocationHeader *header = reinterpret_cast(payload) - 1; > + header->allocated = allocated; > + header->payload_size = payload_size; > + > + return HWY_ASSUME_ALIGNED(reinterpret_cast(payload), kAlignment); > +} > + > +template static T *AllocateAlignedItems(size_t items) { > + constexpr size_t size = sizeof(T); > + > + constexpr bool is_pow2 = (size & (size - 1)) == 0; > + constexpr size_t bits = ShiftCount(size); > + static_assert(!is_pow2 || (1ull << bits) == size, "ShiftCount is incorrect"); > + > + const size_t bytes = is_pow2 ? items << bits : items * size; > + const size_t check = is_pow2 ? bytes >> bits : bytes / size; > + if (check != items) { > + return nullptr; // overflowed > + } > + return static_cast(AllocateAlignedBytes(bytes)); > +} > + > +template > +static AlignedFreeUniquePtr AllocateAligned(const size_t items) { > + return AlignedFreeUniquePtr(AllocateAlignedItems(items), > + AlignedFreer()); > +} > + > +int main() { > + AlignedFreeUniquePtr in_lanes = AllocateAligned(2); > + uint16_t expected_lanes[2]; > + in_lanes[0] = 65535; > + in_lanes[1] = 32767; > + expected_lanes[0] = 65534; > + expected_lanes[1] = 16383; > + hwy::N_EMU128::Vec128 v = Load(in_lanes.get()); > + hwy::N_EMU128::Vec128 actual = MulHigh(v, v); > + { > + auto actual_lanes = AllocateAligned(2); > + Store(actual, actual_lanes.get()); > + const uint8_t *expected_array = > + reinterpret_cast(expected_lanes); > + const uint8_t *actual_array = > + reinterpret_cast(actual_lanes.get()); > + for (size_t i = 0; i < 2; ++i) { > + const uint8_t *expected_ptr = expected_array + i * 2; > + const uint8_t *actual_ptr = actual_array + i * 2; > + if (!BytesEqual(expected_ptr, actual_ptr, 2)) { > + abort(); > + } > + } > + } > +} > diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc > index f582d238984..c9dab217f05 100644 > --- a/gcc/tree-vect-stmts.cc > +++ b/gcc/tree-vect-stmts.cc > @@ -3423,6 +3423,14 @@ vectorizable_call (vec_info *vinfo, > return false; > } > > + if (vect_emulated_vector_p (vectype_in) || vect_emulated_vector_p (vectype_out)) > + { > + if (dump_enabled_p ()) > + dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location, > + "use emulated vector type for call\n"); > + return false; > + } > + > /* FORNOW */ > nunits_in = TYPE_VECTOR_SUBPARTS (vectype_in); > nunits_out = TYPE_VECTOR_SUBPARTS (vectype_out); > -- > 2.27.0 >