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

Introduced LitValue for expanded literal value handling #3945

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Xinlu-Y
Copy link
Collaborator

@Xinlu-Y Xinlu-Y commented Aug 24, 2024

As discussed in #1867, smartAdd and smartSub are currently using literal integer values as SValue to perform simple optimizations. By creating a new datatype LitValue, we could extend support to more simple types of literal values (such as floats, doubles, and strings, which I believe would be useful) for addition and subtraction operations.

However, after making these changes, I noticed that the generated test code for listSlice has been altered. This change seems to occur specifically when the step is a variable and the issue arises from how I'm handling the return value of valueInt in the instances of ValueElim.
image
The problem might stem from how these values are being unpacked and interpreted in the context of listSlice which remains somewhat puzzling to me.

@@ -36,11 +36,11 @@ mkStmtNoEnd = flip stmtFromData Empty

-- | Constructs a value in a stateful context
mkStateVal :: (CommonRenderSym r) => VSType r -> Doc -> SValue r
mkStateVal = valFromData Nothing Nothing
mkStateVal = valFromData Nothing (LitInt 0)
Copy link
Owner

Choose a reason for hiding this comment

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

This is suspicious to me: why is the static value defaulting to 0? (same in other places in this PR)

@@ -381,7 +381,21 @@ instance RenderValue JuliaCode where

instance ValueElim JuliaCode where
valuePrec = valPrec . unJLC
valueInt = valInt . unJLC
valueChar sc = case litVal (unJLC sc) of
Copy link
Owner

Choose a reason for hiding this comment

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

I think you should be able to abstract out the

case litVal x of
   LitChar c -> Just c
   _  -> Nothing

part in a separate function so that it can be re-used across the different renderers

@@ -134,16 +134,26 @@ smartAdd v1 v2 = do
v1' <- v1
v2' <- v2
case (RC.valueInt v1', RC.valueInt v2') of
(Just i1, Just i2) -> litInt (i1 + i2)
_ -> v1 #+ v2
(Just i1,Just i2) -> litInt (i1 + i2)
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if smartAdd should be "more typed", so that this cascade of checking doesn't happen.

@@ -278,7 +288,7 @@ lambda f ps' ex' = do
ps <- sequence ps'
ex <- ex'
let ft = IC.funcType (map (return . variableType) ps) (return $ valueType ex)
valFromData (Just 0) Nothing ft (f ps ex)
valFromData (Just 0) (LitInt 0) ft (f ps ex)
Copy link
Owner

Choose a reason for hiding this comment

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

Here again, why default to 0?

@@ -148,11 +148,15 @@ class RenderValue r where
-- calls.
call :: Maybe Library -> Maybe Doc -> MixedCall r

valFromData :: Maybe Int -> Maybe Integer -> VSType r -> Doc -> SValue r
valFromData :: Maybe Int -> LitValue -> VSType r -> Doc -> SValue r
Copy link
Owner

Choose a reason for hiding this comment

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

The previous type was more precise! Perhaps LitValue is not the best way to go, but instead a synonym for Maybe a would be, where the type could be more precise.

@@ -126,33 +126,31 @@ int main(int argc, const char *argv[]) {
mySlicedList7 = temp5;

vector<double> temp6(0);
for (int i4 = 3; 0 < i4 && i4 <= 3 && z < 0; i4 += z) {
for (int i4 = 3; i4 < 0; i4 += z) {
Copy link
Owner

Choose a reason for hiding this comment

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

I would not be surprised if this change was due to your defaulting some things to 0!

It is also strange that this changed, as you don't have any code for simplifying constraints / boolean expressions. So I really suspect it's the defaults.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These should be the relevant lines of code:

let cond = case mbStepV of
-- If step is a litInt, do a one-sided check
(Just s) -> if s >= 0 then v_i ?< endVal else v_i ?> endVal
Nothing -> case (mbBegV, mbEndV) of
-- If both bounds are litInt's, do a two-sided check.
-- Also, make sure step is in same direction as check.
(Just b, Just e) -> if e >= b
then begVal ?<= v_i ?&& v_i ?< endVal ?&& step' ?> IC.litInt 0
else endVal ?< v_i ?&& v_i ?<= begVal ?&& step' ?< IC.litInt 0
-- If bounds are not litInt's, do both two-sided checks
_ -> begVal ?<= v_i ?&& v_i ?< endVal ?&& step' ?> IC.litInt 0 ?||
endVal ?< v_i ?&& v_i ?<= begVal ?&& step' ?< IC.litInt 0

I believe @JacquesCarette was right: setting values to litInt 0 makes the line 100 case trigger instead of the line 101 case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case, where step is a variable with a negative value, valueInt stepV will return Nothing, making mbStepV = Nothing, which I believe would trigger the logic starting at line 101. However, in this code, how does the default value litInt 0 affect mbStepV and result in mbStepV getting a value of 0, which subsequently generates the code for (int i4 = 3; i4 < 0; i4 += z)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the line creating the listSlice in the example:

-- | List slicing where the step is a variable with negative value
listSlice mySlicedList8
(valueOf myOtherList) (Just (litInt 3))
(Just (litInt 0)) (Just (valueOf (var "z" int))),

I believe that the specific reason that line 100 is triggering instead of line 101 is that the value of step uses valueOf, which gives litInt 0 as its literal value.

Variables aren't literal values; they hold values. Since GOOL doesn't know what the value they're holding is, the correct litVal for them should be Nothing, as they aren't literal values.

@JacquesCarette
Copy link
Owner

Looking at this code again, I don't think this is quite the right approach -- it loses too much type information. I think the 'static' information should be tracked in a different way. Most likely abandoning this PR and re-starting will be the easiest way forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants