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.

Sign up for GitHub

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

Jump to bottom

[Truffle] Remove unused frame parameter in guards. #3694

Closed
wants to merge 1 commit into from
Closed

[Truffle] Remove unused frame parameter in guards. #3694

wants to merge 1 commit into from
+10 −10

Conversation

chrisseaton
Copy link
Contributor

@chumer this modification makes my program fail on this assert in the generated code

    @GeneratedBy(methodName = "writeInteger(Frame, int)", value = WriteFrameSlotNode.class)
    private static final class WriteIntegerNode_ extends BaseNode_ {

        WriteIntegerNode_(WriteFrameSlotNodeGen root) {
            super(root, 2);
        }

        @Override
        public Object execute(Frame frameValue, Object arg0Value) {
            if (arg0Value instanceof Integer) {
                int arg0Value_ = (int) arg0Value;
                assert (root.checkIntegerKind());
                return root.writeInteger(frameValue, arg0Value_);
            }
            return getNext().execute(frameValue, arg0Value);
        }

        static BaseNode_ create(WriteFrameSlotNodeGen root) {
            return new WriteIntegerNode_(root);
        }

    }

With the frame parameter in, the generated code is

    @GeneratedBy(methodName = "writeInteger(Frame, int)", value = WriteFrameSlotNode.class)
    private static final class WriteIntegerNode_ extends BaseNode_ {

        WriteIntegerNode_(WriteFrameSlotNodeGen root) {
            super(root, 2);
        }

        @Override
        public Object execute(Frame frameValue, Object arg0Value) {
            if (arg0Value instanceof Integer) {
                int arg0Value_ = (int) arg0Value;
                if ((root.checkIntegerKind(frameValue))) {
                    return root.writeInteger(frameValue, arg0Value_);
                }
            }
            return getNext().execute(frameValue, arg0Value);
        }

        static BaseNode_ create(WriteFrameSlotNodeGen root) {
            return new WriteIntegerNode_(root);
        }

    }

Do you know what could be wrong? Why is the generated code so different? The frame variable in my code isn't even used.

@chrisseaton chrisseaton added this to the truffle-dev milestone Feb 23, 2016
@eregon
Copy link
Member

eregon commented Feb 24, 2016

The frame or a live value (an argument) must be passed explicitly here, otherwise the guard is considered pure and "cached" and is only ever called once at specialization instantiation time. The consequence is if the frame slot changes, the guard won't catch it and the assert will check that the guard is pure (return the same result), but it is not!

@eregon
Copy link
Member

eregon commented Feb 24, 2016

The SL node does it very similarly, but it does not have a comment about why the Frame is passed.

@eregon
Copy link
Member

eregon commented Feb 24, 2016

Maybe using Frame.isInt(FrameSlot) would be nicer for this use case?

@eregon
Copy link
Member

eregon commented Feb 24, 2016

Maybe using Frame.isInt(FrameSlot) would be nicer for this use case?

Nevermind, that seems to check something different.

@chumer
Copy link
Contributor

chumer commented Feb 24, 2016

Yes Benoit is right. If no dynamic value is in the expression the whole expression is considered compilation constant. I considered introducing an explicit syntax for this when I implemented that feature, but I thought the more common case would be that its considered compilation constant.

@chrisseaton
Copy link
Contributor Author

Ok I understand the issue now.

I think it's a problem because removing dead code shouldn't change behaviour - it even compiles with no apparent difference until you run it - and if your tests don't cover it for whatever reason it's going to be trouble.

Assuming arbitrary code that nobody has annotated in anyway won't have side effects just seems wrong in the first place.

I'm not sure what to suggest though. Are the pure, cached guards a big win in some cases?

@eregon
Copy link
Member

eregon commented Feb 24, 2016

I'm not sure what to suggest though. Are the pure, cached guards a big win in some cases?

Well, yes, otherwise they would need to be checked at every call, even when called on only @Cached values.
It's documented in http://graalvm.github.io/truffle/javadoc/latest/com/oracle/truffle/api/dsl/Specialization.html#guards--

@chrisseaton chrisseaton closed this Mar 1, 2016
@chrisseaton chrisseaton deleted the truffle-weird-dsl branch March 1, 2016 23:09
@enebo enebo added this to the Non-Release milestone Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
in progress
Projects
None yet
Milestone
Non-Release
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@chrisseaton @eregon @chumer @enebo