Codegen can result in multiple sequential mfence instructions

[redbeard0531]

Bugzilla Link	37690
Version	trunk
OS	Linux
CC	@DougGregor,@RKSimon,@rotateright,@TNorthover

Extended Description

This is from an example to prove that atomic_thread_fence does not prevent the compiler from optimizing non-escaped memory.

https://godbolt.org/g/WXFdPL

#include <utility>
#include <atomic>
struct Type {
    Type(Type&&)=default;
    int i;
};

Type func(Type t) {
    auto out = Type(Type(std::move(t)));
    std::atomic_thread_fence(std::memory_order_seq_cst);
    return out;
}

auto func2(Type t) { return func(func(func(func(std::move(t))))); }

func(Type): # @func(Type)
  mfence
  mov eax, edi
  ret
func2(Type): # @func2(Type)
  mfence
  mfence
  mfence
  mfence
  mov eax, edi
  ret

[rotateright]

Seems like an IR optimizer problem?

Note that the extra fences are removed when there's no debug info, so some pass is probably failing to discount those debug instructions:
https://godbolt.org/g/RgmgsW

[TNorthover]

This particular one was InstCombine. I've fixed it in r334090, mostly because of the debug/non-debug difference.

In general there's been some work in this area in the past: the peephole affected here, and a slightly more general attempt on ARM in ARMOptimizeBarriers.cpp. But there's a lot more work that can and should be done to optimize atomic instructions of various kinds, particularly in control-flow sensitive ways.

I can't see an obvious duplicate for the more general optimization request so I'm going to leave this open for now.

[rotateright]

This particular one was InstCombine. I've fixed it in r334090, mostly
because of the debug/non-debug difference.

Thanks, Tim!

In general there's been some work in this area in the past: the peephole
affected here, and a slightly more general attempt on ARM in
ARMOptimizeBarriers.cpp. But there's a lot more work that can and should be
done to optimize atomic instructions of various kinds, particularly in
control-flow sensitive ways.

I can't see an obvious duplicate for the more general optimization request
so I'm going to leave this open for now.

Do we have a source or IR example for that more general case? Unless you're planning to work on this soon, I suspect this bug report will become a zombie without a real test case to solve.

[TNorthover]

Do we have a source or IR example for that more general case?

A couple of obvious ones that would be easy to fix:

    ; Type of fence needn't match exactly. A single seq_cst would be enough.
    define void @foo() {
      fence seq_cst
      fence acq_rel
      ret void
    }

    ; Ops without side-effects don't inhibit folding.
    define i32 @foo(i32 %a) {
      fence seq_cst
      %res = add i32 %a, %a
      fence seq_cst
      ret i32 %res
    }

You could even merge across loads & stores if you know the pointer can't escape (though I don't think it's allowed otherwise).

[redbeard0531]

Is this the same issue or should I file a new bug: https://godbolt.org/g/QcLkce

[rotateright]

Is this the same issue or should I file a new bug:
https://godbolt.org/g/QcLkce

I don't have much experience with atomic ops, but the safe thing is to file another bug and relate to this one. In the worst case, if it ends up being solved with the same fix, someone can mark it as a duplicate.

[TNorthover]

I’d say a separate one too. You could reasonably easily implement at least the cases I mentioned in one commit. Adding fused ops too really would turn this into an “optimise atomic” bug.

Which we might want anyway as a parent, at least if LLVM had sane bug hierarchies. But I think we only really do that for releases.

[redbeard0531]

mentioned in issue llvm/llvm-bugzilla-archive#37716

[rotateright]

mentioned in issue llvm/llvm-bugzilla-archive#37726