Alex Coplan <alex.cop...@arm.com> writes:
> Hi,
>
> As the PR shows, pair-fusion was tricking memory_modified_in_insn_p into
> returning false when a common base register (in this case, x1) was
> modified between the mem and the store insn. This lead to wrong code as
> the accesses really did alias.
>
> To avoid this sort of problem, this patch avoids invoking RTL alias
> analysis altogether (and assume an alias conflict) if the two insns to
> be compared share a common address register R, and the insns see different
> definitions of R (i.e. it was modified in between).
>
> Bootstrapped/regtested on aarch64-linux-gnu (all languages, both regular
> bootstrap and LTO+PGO bootstrap). OK for trunk?
Sorry for the slow review. The patch looks good to me, but...
> @@ -2544,11 +2624,37 @@ pair_fusion_bb_info::try_fuse_pair (bool load_p,
> unsigned access_size,
> && bitmap_bit_p (&m_tombstone_bitmap, insn->uid ());
> };
>
> + // Maximum number of distinct regnos we expect to appear in a single
> + // MEM (and thus in a candidate insn).
> + static constexpr int max_mem_regs = 2;
> + auto_vec<access_info *, max_mem_regs> addr_use_vec[2];
> + use_array addr_uses[2];
> +
> + // Collect the lists of register uses that occur in the candidate MEMs.
> + for (int i = 0; i < 2; i++)
> + {
> + // N.B. it's safe for us to ignore uses that only occur in notes
> + // here (e.g. in a REG_EQUIV expression) since we only pass the
> + // MEM down to the alias machinery, so it can't see any insn-level
> + // notes.
> + for (auto use : insns[i]->uses ())
> + if (use->is_reg ()
> + && use->includes_address_uses ()
> + && !use->only_occurs_in_notes ())
> + {
> + gcc_checking_assert (addr_use_vec[i].length () < max_mem_regs);
> + addr_use_vec[i].quick_push (use);
...if possible, I think it would be better to just use safe_push here,
without the assert. There'd then be no need to split max_mem_regs out;
it could just be hard-coded in the addr_use_vec declaration.
Or does that not work for some reason? I'm getting a sense of deja vu...
If it doesn't work, an alternative would be to use access_array_builder.
OK for trunk and backports if using safe_push works.
Thanks,
Richard
> + }
> + addr_uses[i] = use_array (addr_use_vec[i]);
> + }
> +
> store_walker<false, decltype(tombstone_p)>
> - forward_store_walker (mem_defs[0], cand_mems[0], insns[1], tombstone_p);
> + forward_store_walker (mem_defs[0], cand_mems[0], addr_uses[0], insns[1],
> + tombstone_p);
>
> store_walker<true, decltype(tombstone_p)>
> - backward_store_walker (mem_defs[1], cand_mems[1], insns[0], tombstone_p);
> + backward_store_walker (mem_defs[1], cand_mems[1], addr_uses[1], insns[0],
> + tombstone_p);
>
> alias_walker *walkers[4] = {};
> if (mem_defs[0])
> @@ -2562,8 +2668,10 @@ pair_fusion_bb_info::try_fuse_pair (bool load_p,
> unsigned access_size,
> {
> // We want to find any loads hanging off the first store.
> mem_defs[0] = memory_access (insns[0]->defs ());
> - load_walker<false> forward_load_walker (mem_defs[0], insns[0],
> insns[1]);
> - load_walker<true> backward_load_walker (mem_defs[1], insns[1],
> insns[0]);
> + load_walker<false> forward_load_walker (mem_defs[0], insns[0],
> + addr_uses[0], insns[1]);
> + load_walker<true> backward_load_walker (mem_defs[1], insns[1],
> + addr_uses[1], insns[0]);
> walkers[2] = &forward_load_walker;
> walkers[3] = &backward_load_walker;
> m_pass->do_alias_analysis (alias_hazards, walkers, load_p);
> diff --git a/gcc/testsuite/g++.dg/torture/pr116783.C
> b/gcc/testsuite/g++.dg/torture/pr116783.C
> new file mode 100644
> index 00000000000..6d59159459d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/torture/pr116783.C
> @@ -0,0 +1,98 @@
> +// { dg-do run }
> +// { dg-additional-options "-fstack-protector-strong
> -fno-late-combine-instructions" }
> +// { dg-require-effective-target fstack_protector }
> +// { dg-require-effective-target c++11 }
> +
> +struct Private {
> + char data[24]{};
> + long moved_from : 4;
> + Private() : moved_from (0) {}
> +};
> +
> +struct QVariant {
> + __attribute__((noipa))
> + ~QVariant() {
> + if (!d.moved_from && d.data[0] != 42)
> + __builtin_abort ();
> + }
> + __attribute__((noipa))
> + QVariant() {
> + d.data[0] = 42;
> + }
> + __attribute__((noipa))
> + QVariant(QVariant &other) : d(other.d) {}
> + QVariant(QVariant &&other) : d(other.d) {
> + other.d = Private();
> + other.d.moved_from = true;
> + }
> + QVariant &operator=(QVariant);
> + Private d;
> +};
> +
> +QVariant id (QVariant v) { return v; }
> +QVariant &QVariant::operator=(QVariant other)
> +{
> + id(other);
> + return *this;
> +}
> +
> +template <typename T> struct QList {
> + T d;
> + struct const_iterator {
> + T *ptr;
> + T &operator*() { return *ptr; }
> + __attribute__((noipa))
> + bool operator!=(const_iterator other) { return ptr != other.ptr; }
> + void operator++() { ptr++; }
> + };
> + __attribute__((noipa))
> + T at() { return d; }
> + const_iterator begin() { return const_iterator { &d }; }
> + const_iterator end() { return const_iterator { &d }; }
> +};
> +struct QArrayDataPointer {
> + int d;
> + int *ptr;
> + long size;
> +};
> +
> +QArrayDataPointer null_qadp;
> +
> +struct QString {
> + __attribute__((noipa))
> + QList<QString> split() const {
> + return QList<QString> {null_qadp};
> + }
> + __attribute__((noipa))
> + friend bool operator==(QString, QString) { return true; }
> +
> + __attribute__((noipa))
> + QString(QArrayDataPointer dp) : d(dp) {}
> + QArrayDataPointer d;
> +};
> +
> +__attribute__((noipa))
> +QString as_qstr (QVariant *v)
> +{
> + return QString (null_qadp);
> +}
> +
> +int *getNode(const QString &tagContent) {
> + auto expr = tagContent.split();
> + auto blockName = expr.at();
> + auto loadedBlocksVariant = QVariant ();
> + QList<QVariant> blockVariantList;
> + for (auto &item : blockVariantList) {
> + auto blockNodeName = as_qstr (&item);
> + blockNodeName == blockName;
> + QString q(null_qadp);
> + }
> + loadedBlocksVariant = QVariant();
> + return nullptr;
> +}
> +
> +int main(void)
> +{
> + QString foo(null_qadp);
> + getNode(foo);
> +}
