Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check consistency of constraints before evaluating a pattern #4013

Closed
wants to merge 25 commits into from
Closed
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
99988c1
implment isSat
goodlyrottenapple Jun 27, 2024
f368038
Check constraints for consistency in evaluatePattern'
geo2a Aug 1, 2024
32b08b7
Do not fail on Unknown pattern constraints
geo2a Aug 1, 2024
59bd44b
Ignore prelude check result when running with noSolver
geo2a Aug 1, 2024
fd3ea50
Set FEATURE_SERVER_OPTS to '' if it's unbound
geo2a Aug 1, 2024
bb6a765
Emit more logs from evaluatePattern'
geo2a Aug 1, 2024
29524b8
Check consistency of the pattern before rewriting
geo2a Aug 1, 2024
8852282
Move log message out
geo2a Aug 2, 2024
8f13183
Remove bad hardResetSolver
geo2a Aug 2, 2024
1948b15
Hard reset in the beginning of isSat
geo2a Aug 2, 2024
8bef12a
Update integration test output
geo2a Aug 2, 2024
432ff02
pretty: print substitution as bindings, separate imports
geo2a Aug 5, 2024
7fe9293
Update outputs of booster/test/rpc-integration/test-substitution
geo2a Aug 5, 2024
2483a57
Run test-substitutions with booster-dev
geo2a Aug 5, 2024
25afc73
Use the same code to update SMT options on retries
geo2a Aug 5, 2024
c9f107d
Factor-out and unify failBecauseUnknown
geo2a Aug 5, 2024
2ef21ab
Update comments
geo2a Aug 5, 2024
64147e3
Factor-out duration
geo2a Aug 5, 2024
24b8126
comment
geo2a Aug 5, 2024
10f6b18
Pass initial simplifier cache to performRewrite
geo2a Aug 5, 2024
7f7d667
Correct comments
geo2a Aug 5, 2024
80d1675
Fix unit tests
geo2a Aug 5, 2024
1fce254
Use record field punning to access the constraints
geo2a Aug 6, 2024
d82576f
Improve control-flow in evaluatePattern'
geo2a Aug 6, 2024
575428c
Rename test case
geo2a Aug 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 56 additions & 31 deletions booster/library/Booster/JsonRpc.hs
Original file line number Diff line number Diff line change
Expand Up @@ -155,33 +155,51 @@ respond stateVar request =

solver <- maybe (SMT.noSolver) (SMT.initSolver def) mSMTOptions

logger <- getLogger
prettyModifiers <- getPrettyModifiers
let rewriteConfig =
RewriteConfig
{ definition = def
, llvmApi = mLlvmLibrary
, smtSolver = solver
, varsToAvoid = substVars
, doTracing
, logger
, prettyModifiers
, mbMaxDepth = mbDepth
, mbSimplify = rewriteOpts.interimSimplification
, cutLabels = cutPoints
, terminalLabels = terminals
}
result <-
performRewrite rewriteConfig substPat
SMT.finaliseSolver solver
stop <- liftIO $ getTime Monotonic
let duration =
if fromMaybe False req.logTiming
then
Just $
fromIntegral (toNanoSecs (diffTimeSpec stop start)) / 1e9
else Nothing
pure $ execResponse duration req result substitution unsupported
-- check input pattern's consistency before starting rewriting
evaluatedInitialPattern <-
ApplyEquations.evaluatePattern
def
mLlvmLibrary
solver
mempty
substPat

case evaluatedInitialPattern of
(Left ApplyEquations.SideConditionFalse{}, _) -> do
-- input pattern's constraints are Bottom, return Vacuous
stop <- liftIO $ getTime Monotonic
pure $
execResponse
(duration req.logTiming start stop)
req
(0, mempty, RewriteTrivial substPat)
substitution
unsupported
(Left other, _) ->
pure . Left . RpcError.backendError $ RpcError.Aborted (Text.pack . constructorName $ other)
(Right newPattern, simplifierCache) -> do
logger <- getLogger
prettyModifiers <- getPrettyModifiers
let rewriteConfig =
RewriteConfig
{ definition = def
, llvmApi = mLlvmLibrary
, smtSolver = solver
, varsToAvoid = substVars
, doTracing
, logger
, prettyModifiers
, mbMaxDepth = mbDepth
, mbSimplify = rewriteOpts.interimSimplification
, cutLabels = cutPoints
, terminalLabels = terminals
}

result <-
performRewrite rewriteConfig simplifierCache newPattern
SMT.finaliseSolver solver
stop <- liftIO $ getTime Monotonic
pure $ execResponse (duration req.logTiming start stop) req result substitution unsupported
RpcTypes.AddModule RpcTypes.AddModuleRequest{_module, nameAsId = nameAsId'} -> Booster.Log.withContext CtxAddModule $ runExceptT $ do
-- block other request executions while modifying the server state
state <- liftIO $ takeMVar stateVar
Expand Down Expand Up @@ -247,9 +265,9 @@ respond stateVar request =
start <- liftIO $ getTime Monotonic
let internalised =
runExcept $ internaliseTermOrPredicate DisallowAlias CheckSubsorts Nothing def req.state.term
let mkTraces duration
let mkTraces durationLog
| Just True <- req.logTiming =
Just [ProcessingTime (Just Booster) duration]
Just [ProcessingTime (Just Booster) durationLog]
Comment on lines -250 to +270
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

o/t ...maybe we should just remove all code for log-timing...
What I am using is the proxy-level timing log, not this level underneath.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've prepared a separate PR that removes "log-timing" and "log-fallbacks": #4015

| otherwise =
Nothing

Expand Down Expand Up @@ -327,11 +345,11 @@ respond stateVar request =
SMT.finaliseSolver solver
stop <- liftIO $ getTime Monotonic

let duration =
let durationLog =
fromIntegral (toNanoSecs (diffTimeSpec stop start)) / 1e9
mkSimplifyResponse state =
RpcTypes.Simplify
RpcTypes.SimplifyResult{state, logs = mkTraces duration}
RpcTypes.SimplifyResult{state, logs = mkTraces durationLog}
pure $ second mkSimplifyResponse result
RpcTypes.GetModel req -> withModule req._module $ \case
(_, _, Nothing, _) -> do
Expand Down Expand Up @@ -567,6 +585,13 @@ respond stateVar request =
, logs = Nothing
}

duration mLogTiming start stop =
if fromMaybe False mLogTiming
then
Just $
fromIntegral (toNanoSecs (diffTimeSpec stop start)) / 1e9
else Nothing

handleSmtError :: JsonRpcHandler
handleSmtError = JsonRpcHandler $ \case
SMT.GeneralSMTError err -> runtimeError "problem" err
Expand Down
52 changes: 44 additions & 8 deletions booster/library/Booster/Pattern/ApplyEquations.hs
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,11 @@ evaluateTerm' ::
evaluateTerm' direction = iterateEquations direction PreferFunctions

{- | Simplify a Pattern, processing its constraints independently.
Returns either the first failure or the new pattern if no failure was encountered

Before evaluating the term of the pattern,
the constraints of the pattern are checked for consistency with an SMT solver.

Returns either the first failure or the new pattern if no failure was encountered.
-}
evaluatePattern ::
LoggerMIO io =>
Expand All @@ -458,14 +462,43 @@ evaluatePattern' ::
Pattern ->
EquationT io Pattern
evaluatePattern' pat@Pattern{term, ceilConditions} = withPatternContext pat $ do
newTerm <- withTermContext term $ evaluateTerm' BottomUp term `catch_` keepTopLevelResults
-- after evaluating the term, evaluate all (existing and
-- newly-acquired) constraints, once
traverse_ simplifyAssumedPredicate . predicates =<< getState
-- this may yield additional new constraints, left unevaluated
evaluatedConstraints <- predicates <$> getState
pure Pattern{constraints = evaluatedConstraints, term = newTerm, ceilConditions}
solver <- (.smtSolver) <$> getConfig
-- check the pattern's constraints for consistency, reporting an error if they are Bottom
withContext CtxConstraint
. withContext CtxDetail
. withTermContext (coerce $ collapseAndBools pat.constraints)
$ pure ()
consistent <- withContext CtxConstraint $ SMT.isSat solver pat.constraints
withContext CtxConstraint $ do
logMessage $
"Constraints consistency check returns: " <> show consistent

case consistent of
Right False -> do
-- the constraints are unsatisfiable, which means that the patten is Bottom
throw . SideConditionFalse . collapseAndBools $ pat.constraints
Left SMT.SMTSolverUnknown{} -> do
-- unlikely case of an Unknown response to a consistency check.
-- continue to preserver the old behaviour.
withContext CtxConstraint . logWarn . Text.pack $
"Constraints consistency UNKNOWN: " <> show consistent
continue
Left other ->
-- fail hard on SMT error other than @SMT.SMTSolverUnknown@
liftIO $ Exception.throw other
Right True -> do
-- constraints are consistent, continue
continue
where
continue = do
newTerm <- withTermContext term $ evaluateTerm' BottomUp term `catch_` keepTopLevelResults
-- after evaluating the term, evaluate all (existing and
-- newly-acquired) constraints, once
traverse_ simplifyAssumedPredicate . predicates =<< getState
-- this may yield additional new constraints, left unevaluated
evaluatedConstraints <- predicates <$> getState
pure Pattern{constraints = evaluatedConstraints, term = newTerm, ceilConditions}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is debatable whether we should give the SMT solver the original (unevaluated) pat.constraints here.
As discussed, we should (eventually) keep the originals around but they might contain unevaluated function calls that the solver would abstract over. AFAICT it is still a sound approximation, though, the solver might just not recognise existing contradictions because it abstracts them away.

And a few stylistic things to note:

  • (minor) code mixes record-dot (pat.constraints) and field pun match (pat{term, ceilConditions} access
  • the continue construction is unnecessary if we anyway throw exceptions or just go on with a warning
  • Repeated withContext CtxConstraint could be gathered
Suggested change
withContext CtxConstraint
. withContext CtxDetail
. withTermContext (coerce $ collapseAndBools pat.constraints)
$ pure ()
consistent <- withContext CtxConstraint $ SMT.isSat solver pat.constraints
withContext CtxConstraint $ do
logMessage $
"Constraints consistency check returns: " <> show consistent
case consistent of
Right False -> do
-- the constraints are unsatisfiable, which means that the patten is Bottom
throw . SideConditionFalse . collapseAndBools $ pat.constraints
Left SMT.SMTSolverUnknown{} -> do
-- unlikely case of an Unknown response to a consistency check.
-- continue to preserver the old behaviour.
withContext CtxConstraint . logWarn . Text.pack $
"Constraints consistency UNKNOWN: " <> show consistent
continue
Left other ->
-- fail hard on SMT error other than @SMT.SMTSolverUnknown@
liftIO $ Exception.throw other
Right True -> do
-- constraints are consistent, continue
continue
where
continue = do
newTerm <- withTermContext term $ evaluateTerm' BottomUp term `catch_` keepTopLevelResults
-- after evaluating the term, evaluate all (existing and
-- newly-acquired) constraints, once
traverse_ simplifyAssumedPredicate . predicates =<< getState
-- this may yield additional new constraints, left unevaluated
evaluatedConstraints <- predicates <$> getState
pure Pattern{constraints = evaluatedConstraints, term = newTerm, ceilConditions}
withContext CtxConstraint $ do
consistent <-
withContext CtxDetail . withTermContext (coerce $ collapseAndBools pat.constraints) $
SMT.isSat solver pat.constraints
logMessage $ "Constraints consistency check returns: " <> show consistent
case consistent of
Right False -> do
-- the constraints are unsatisfiable, which means that the patten is Bottom
throw . SideConditionFalse . collapseAndBools $ pat.constraints
Left SMT.SMTSolverUnknown{} -> do
-- unlikely case of an Unknown response to a consistency check.
-- continue to preserver the old behaviour.
logWarn . Text.pack $ "Constraints consistency UNKNOWN: " <> show consistent
Left other ->
-- fail hard on SMT error other than @SMT.SMTSolverUnknown@
liftIO $ Exception.throw other
Right True -> -- constraints are consistent, continue
pure ()
newTerm <- withTermContext term $ evaluateTerm' BottomUp term `catch_` keepTopLevelResults
-- after evaluating the term, evaluate all (existing and
-- newly-acquired) constraints, once
traverse_ simplifyAssumedPredicate . predicates =<< getState
-- this may yield additional new constraints, left unevaluated
evaluatedConstraints <- predicates <$> getState
pure Pattern{constraints = evaluatedConstraints, term = newTerm, ceilConditions}

Copy link
Collaborator Author

@geo2a geo2a Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • after discussing in the daily meeting, we have decided to proceed with checking the constrains as is
  • I've introduced some stylistic corrections --- thanks!

-- when TooManyIterations exception occurred while evaluating the top-level term,
-- i.e. not in a recursive evaluation of a side-condition,
-- it is safe to keep the partial result and ignore the exception.
Expand All @@ -477,6 +510,9 @@ evaluatePattern' pat@Pattern{term, ceilConditions} = withPatternContext pat $ do
pure partialResult
err -> throw err

collapseAndBools :: Set Predicate -> Predicate
collapseAndBools = coerce . foldAndBool . map coerce . Set.toList

-- evaluate the given predicate assuming all others
simplifyAssumedPredicate :: LoggerMIO io => Predicate -> EquationT io ()
simplifyAssumedPredicate p = do
Expand Down
19 changes: 10 additions & 9 deletions booster/library/Booster/Pattern/Rewrite.hs
Original file line number Diff line number Diff line change
Expand Up @@ -704,9 +704,10 @@ performRewrite ::
forall io.
LoggerMIO io =>
RewriteConfig ->
SimplifierCache ->
Pattern ->
io (Natural, Seq (RewriteTrace ()), RewriteResult Pattern)
performRewrite rewriteConfig pat = do
performRewrite rewriteConfig initialCache pat = do
(rr, RewriteStepsState{counter, traces}) <-
flip runStateT rewriteStart $ doSteps False pat
pure (counter, traces, rr)
Expand All @@ -722,6 +723,14 @@ performRewrite rewriteConfig pat = do
, terminalLabels
} = rewriteConfig

rewriteStart :: RewriteStepsState
rewriteStart =
RewriteStepsState
{ counter = 0
, traces = mempty
, simplifierCache = initialCache
}

logDepth = withContext CtxDepth . logMessage

depthReached n = maybe False (n >=) mbMaxDepth
Expand Down Expand Up @@ -919,11 +928,3 @@ data RewriteStepsState = RewriteStepsState
, traces :: !(Seq (RewriteTrace ()))
, simplifierCache :: SimplifierCache
}

rewriteStart :: RewriteStepsState
rewriteStart =
RewriteStepsState
{ counter = 0
, traces = mempty
, simplifierCache = mempty
}
Loading
Loading