matsc-at-sics-se / unison

Unison's source code
http://unison-code.github.io/
Other
5 stars 0 forks source link

Unison.Tools.Import.TagRemats: some filter needed #8

Closed matsc-at-sics-se closed 6 years ago

matsc-at-sics-se commented 6 years ago

Too many temps/operations are tagged rematerializable, for example:

spio_mutex_debug_is_unlocked_pthread.uni:

[...]
    o34: [t50] <- SETEr []
[...]
rematerializable:
    t21 [o11], t35 [o17], t46 [o27], t50 [o34]
[...]

But this comes from:

%13 = SETEr implicit %eflags

so it is affected by eflags.

Also frequently tagged as rematerializable is is MOV64rm, which moves from memory to register.

Unison/Graphs/Hoopl/ReachingConstantsSSA.hs:

middleConstants :: Ord i => Show i => Ord r => Show r => HOperation i r O O ->
                   ReachingConstants i r -> ReachingConstants i r
middleConstants (HMiddle o) consts =
    let ds      = concatMap extractTemps (oDefs o)
        e       = if isRematerializable o && length ds == 1
                  then mkLatticeElement consts o
                  else ConstBottom
        new     = M.fromList $ zip ds (repeat e)
        consts' = joinFacts new consts
    in consts'

For spio_mutex_debug_is_unlocked_pthread, I witnessed that code making a lattice element:

ConstOpr {constOpr = o0: [t0] <- SETEr [], constDefs = fromList [34]}

which should have been suppressed by some test.

robcasloz commented 6 years ago

Unison leaves the final decision of whether and how rematerialization candidates are implemented to the target which has the precise knowledge about side-effects, etc. Just return Nothing for these instructions in the target description function tRematInstrs.

matsc-at-sics-se commented 6 years ago

I'm doing that, but it would seem cleaner to do it more upstream.

SETEr has a register dependency (%eflags), MOV64rm has a memory dependency. Such dependencies seem pretty generic to me. But having spurious operations marked rematerializable does not cause any problem downstream.