In D36562#1641930, @wmi wrote:

In D36562#1639441, @chill wrote:

Shouldn't we disable OPT_ffine_grained_bitfield_accesses only if TSAN is active?

I don't remember why it is disabled for all sanitizer modes. Seems you are right that the disabling the option is only necessary for TSAN. Do you have actual needs for the option to be functioning on other sanitizer modes?

Shouldn't we disable OPT_ffine_grained_bitfield_accesses only if TSAN is active?

Could you, please, give some code examples of the issues that you see?

Rebase on top of master.

If there are no objections, I'll go ahead with committing that Soon(tm) on the basis of previous acceptance.

Done, thanks!

Ping?

Changed tcancel implementation.

I reverted the patch, have to rework tcancel.

Going to commit as obvious.

Added a regression test.

In D64414#1578661, @jdoerfert wrote:

In D64414#1577844, @chill wrote:

In D64414#1576955, @jdoerfert wrote:

What if a different thread causes an abort before the commit? store-load forwarding would then cause an inconsistent view, wouldn't it?
I find this is like atomic loads that also "write" memory if the synchronization is strong enough to make other modifications visible at that point.

If the transaction is aborted before or by the tcommit, it'll be automatically rolled back to the tstart, which initiated it, discarding
any register and memory writes, and won't reach the instruction after the tcommit.

I agree, that is what needs to happen. Maybe I misunderstood what your example was supposed to show. The way I did understand it, I figured you want the store to load propagation to happen across the tcommit instruction. That has to be forbidden in the IR as it will otherwise happen and break the atomicity of the transaction. To forbid it one would give the tcommit intrinsic a memory effect. Now, I think it actually has a memory effect: Similar to sequential consistent atomic loads, tcommit synchronizes, thereby making memory changes visible to the thread.

We can move the discussion to the main patch if you want.

Thanks for the review. I've addressed the comments.

I suggested this change here: https://reviews.llvm.org/D60486#inline-574695
But I've got the feeling (from https://reviews.llvm.org/D64414) it a subject for a debate.

In D64414#1576955, @jdoerfert wrote:

What if a different thread causes an abort before the commit? store-load forwarding would then cause an inconsistent view, wouldn't it?
I find this is like atomic loads that also "write" memory if the synchronization is strong enough to make other modifications visible at that point.

In D64414#1576588, @jdoerfert wrote:

In D64414#1576554, @chill wrote:

In D64414#1576252, @jdoerfert wrote:

I'm a bit lost. What intrinsic do we have that is "no-mem" but has side-effects?

E.g __builtin_arm_tcommit, which I'm adding in this series, or __builtin_arm_irg (which is *should* be NoMem, but is not, perhaps as a workaround for this issue).

(Sorry if I'm way off here but I'd like to understand this better)

tcommit makes the transaction state visible, correct? If so, I'd argue it modifies the memory (especially for other threads). Maybe my confusion stems from the IR vs MIR conflict here but in IR, I would not like tcommit to be readnone for multiple reasons, including memory movement around it.

In D64414#1576252, @jdoerfert wrote:

I'm a bit lost. What intrinsic do we have that is "no-mem" but has side-effects?

In D64414#1575894, @lebedev.ri wrote:

If an intrinsic is defined without outputs, but having side effects, it still can be removed completely from the program

This kinda sounds slightly backwards.
*If* there are side-effects, then i'd expect it not to be dropped regardless of readnone status.

Sorry, messages crossed.

LGTM, that's what EHABI spec says:

In D64414#1575868, @lebedev.ri wrote:

Test?