Lars T Hansen [:lth] Assignee
	Description • 9 years ago

See bug 1147810, especially bug 1147810 comment 7:

Atomic ops (load, store, compareExchange, add, sub, and, or, xor) are flagged as non-movable (their "movable" flag is not set) and non-removable-even-if-not-used (their "guarded" flag is set) and all have a "Store" alias set.

Are those enough to guarantee that loads and stores are not:
 - commoned?
 - reordered with respect to normal loads and stores on shared memory?

In bug 1147810, it becomes clear that the Store alias set is necessary on atomic load operations to prevent loads from being incorrectly commoned.  That's surprising since a commoning operation is in a sense both moving and removing an operation.  So the questions become, which flags exactly must be set to guarantee the behavior we want, and can we hope to preserve that behavior even in the face of new optimizations or should we add a flag or other feature so as to explicitly ask for behavior required from atomic ops, and if so what should that flag or feature be?

On the flip side, even though the optimizer does not currently remove redundant stores -- consider two consecutive stores to provably the same location -- it could do that, and it could do that even for atomics in some cases.  It's probably not /important/ for atomics to be able to do that, but we don't want to stop the regular optimizer from doing it.

	Jan de Mooij [:jandem]
	Comment 1 • 9 years ago

(In reply to Lars T Hansen [:lth] from comment #0)
> In bug 1147810, it becomes clear that the Store alias set is necessary on
> atomic load operations to prevent loads from being incorrectly commoned. 

If you don't override congruentTo, we should never common the loads. If the Movable flag is set (setMovable), we can use LICM. If the Guard flag is set, we won't DCE the instruction (but this doesn't stop us from using GVN/LICM based on congruentTo/isMovable).

	Lars T Hansen [:lth] Assignee
	Comment 2 • 9 years ago

(In reply to Jan de Mooij [:jandem] from comment #1)
> (In reply to Lars T Hansen [:lth] from comment #0)
> > In bug 1147810, it becomes clear that the Store alias set is necessary on
> > atomic load operations to prevent loads from being incorrectly commoned. 
> 
> If you don't override congruentTo, we should never common the loads. If the
> Movable flag is set (setMovable), we can use LICM. If the Guard flag is set,
> we won't DCE the instruction (but this doesn't stop us from using GVN/LICM
> based on congruentTo/isMovable).

I see.  So it may be necessary to make sure congruentTo defers to the default behavior when barriers are present on load and store.

(It may also soon be time to split atomic load and store out as their own mir nodes.)

	Lars T Hansen [:lth] Assignee
	Comment 3 • 9 years ago

I just checked the code, and congruentTo contains the necessary check that prevents commoning.  It even looks like I added it myself, when the code landed originally.