Page MenuHomePhabricator

[WebAssembly] Support for atomic fences
Needs ReviewPublic

Authored by aheejin on Aug 3 2018, 3:48 PM.

Details

Summary

This adds support for translation of LLVM IR fence instruction. We
convert a singlethread fence to a pseudo compiler barrier which becomes
0 instructions in final binary, and a thread fence to an idempotent
atomicrmw instruction to a memory address.

Diff Detail

Event Timeline

aheejin created this revision.Aug 3 2018, 3:48 PM
aheejin edited the summary of this revision. (Show Details)Aug 3 2018, 3:48 PM
aheejin updated this revision to Diff 159115.Aug 3 2018, 3:52 PM

comment fix

jfb added a comment.Aug 3 2018, 4:01 PM

What do you expect for relaxed, as well as for signal fences?
Could you also check in with Conrad, since he's been working on the WebAssembly memory model?

aheejin added a comment.EditedAug 3 2018, 4:38 PM

What do you expect for relaxed, as well as for signal fences?

Here I actually translated both a signal fence and a thread fence to the same thing. I think this will be conservatively correct, but maybe we should treat them differently. What if we translate a signal fence to a nothing but prevent reordering of instructions across it, and we might do the same thing for asm volatile("" ::: "memory") too. That can be something like making a pseudo instruction that eventually becomes nothing and prevents reordering across the pseudo instruction in the backend passes. What do you think?

Oh and for weaker fences, I added tests for them. They all translate to the same sequentially consistent atomicrmw or instruction.

Could you also check in with Conrad, since he's been working on the WebAssembly memory model?

I don't think Conrad is in the LLVM Phabricator here. Should we move the discussion to a github issue?

aheejin planned changes to this revision.Aug 14 2018, 5:08 PM
aheejin updated this revision to Diff 160909.Aug 15 2018, 1:31 PM
  • Add support for singlethread fences
  • Fixed handling of MachineMemOperand according to changes in rL339740
aheejin edited the summary of this revision. (Show Details)Aug 15 2018, 1:31 PM
aheejin updated this revision to Diff 160941.Aug 15 2018, 4:33 PM

Don't emit the pseudo compiler_fence instruction in .s file

aheejin updated this revision to Diff 161359.Aug 17 2018, 4:24 PM

Use a target external symbol with MO_SYMBOL_GLOBAL

We discussed how to implement fences here, and I think the general consensus was even though it may not be strictly necessary to emit atomicrmw for some cases, it can be user friendly and also might help support legacy builtins like __sync_synchronize(). And IMHO it is a single instruction anyway and fences are generally expected to be expensive, so no significant harm will be done. Does anyone have more concerns on this? If not, can we land this?

We discussed how to implement fences here, and I think the general consensus was even though it may not be strictly necessary to emit atomicrmw for some cases, it can be user friendly and also might help support legacy builtins like __sync_synchronize(). And IMHO it is a single instruction anyway and fences are generally expected to be expensive, so no significant harm will be done. Does anyone have more concerns on this? If not, can we land this?

I have concerns which I expressed here: https://github.com/WebAssembly/tool-conventions/issues/59#issuecomment-413620669

aheejin updated this revision to Diff 186176.Feb 10 2019, 7:43 PM
  • Fix bug: 0 should be i32.const 0 and not an immediate
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2019, 7:43 PM
aheejin updated this revision to Diff 187186.Feb 17 2019, 5:45 PM
  • Use ATOMIC_NRI for the fence + test fix

What is the status of the discussion here? We've gotten to the point where I have to manually add this patch to my local checkout in order to debug unrelated issues and it looks like the conversation fizzled out a while ago, so it would be great if we could land something.