Skip to content

Commit ddfa3ff

Browse files
author
Pavel Marek
committed
Tackle comments in first review round.
1 parent 3cd7502 commit ddfa3ff

File tree

16 files changed

+118
-288
lines changed

16 files changed

+118
-288
lines changed

‎com.oracle.truffle.r.engine/src/com/oracle/truffle/r/engine/REngine.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ public void activate(REnvironment.ContextStateImpl stateREnvironment) {
174174

175175
private void initializeNonShared() {
176176
suppressWarnings = true;
177-
MaterializedFrame baseFrame = RRuntime.createNonFunctionFrameNew("base");
177+
MaterializedFrame baseFrame = RRuntime.createNonFunctionFrame("base");
178178
REnvironment.baseInitialize(baseFrame, globalFrame);
179179
context.getStateRFFI().initializeVariables(context);
180180
RBuiltinPackages.loadBase(context, baseFrame);

‎com.oracle.truffle.r.nodes.builtin/src/com/oracle/truffle/r/nodes/builtin/base/EnvFunctions.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@
3535
import static com.oracle.truffle.r.runtime.builtins.RBuiltinKind.PRIMITIVE;
3636

3737
import com.oracle.truffle.api.CompilerDirectives;
38-
import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
3938
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
4039
import com.oracle.truffle.api.RootCallTarget;
4140
import com.oracle.truffle.api.dsl.Cached;
@@ -606,8 +605,8 @@ public abstract static class MakeActiveBinding extends RBuiltinNode.Arg3 {
606605
casts.arg("env").mustBe(REnvironment.class, Message.NOT_AN_ENVIRONMENT);
607606
}
608607

609-
@CompilationFinal private BranchProfile frameSlotInvalidateProfile = BranchProfile.create();
610-
@CompilationFinal private ValueProfile frameDescriptorProfile = ValueProfile.createIdentityProfile();
608+
private final BranchProfile frameSlotInvalidateProfile = BranchProfile.create();
609+
private final ValueProfile frameDescriptorProfile = ValueProfile.createIdentityProfile();
611610

612611
@TruffleBoundary
613612
@Specialization

‎com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/RASTBuilder.java‎

Lines changed: 9 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,6 @@
8282
import com.oracle.truffle.r.runtime.nodes.RSyntaxLookup;
8383
import com.oracle.truffle.r.runtime.nodes.RSyntaxNode;
8484
import com.oracle.truffle.r.runtime.parsermetadata.FunctionScope;
85-
import com.oracle.truffle.r.runtime.parsermetadata.LocalVariable;
8685

8786
/**
8887
* This class can be used to build fragments of Truffle AST that correspond to R language
@@ -345,28 +344,10 @@ public RootCallTarget rootFunction(TruffleRLanguage language, SourceSection sour
345344

346345
// Local variables
347346
logger.fine(() -> String.format("rootFunction('%s'): functionScope = %s", name, functionScope != null ? functionScope.toString() : "null"));
348-
String[] identifiers;
349-
FrameSlotKind[] slotKinds;
350-
if (functionScope != null && functionScope.getLocalVariablesCount() > 0) {
351-
// Sort local variables by frame index - we have to put them into a FrameDescriptor in a
352-
// specific order.
353-
List<LocalVariable> sortedLocalVariables = functionScope.getLocalVariablesSortedByFrameIdx();
354-
identifiers = new String[sortedLocalVariables.size()];
355-
slotKinds = new FrameSlotKind[sortedLocalVariables.size()];
356-
int currentFrameIdx = FrameSlotChangeMonitor.INTERNAL_INDEXED_SLOT_COUNT;
357-
for (int i = 0; i < sortedLocalVariables.size(); i++) {
358-
LocalVariable localVariable = sortedLocalVariables.get(i);
359-
assert currentFrameIdx == localVariable.getFrameIndex() : "frame indexes of local variables should be ascending";
360-
identifiers[i] = localVariable.getName();
361-
slotKinds[i] = localVariable.getSlotKind();
362-
currentFrameIdx++;
363-
}
364-
} else {
365-
identifiers = new String[]{};
366-
slotKinds = new FrameSlotKind[]{};
367-
}
368-
369-
FrameDescriptor descriptor = FrameSlotChangeMonitor.createFunctionFrameDescriptor(name != null && !name.isEmpty() ? name : "<function>", slotKinds, identifiers);
347+
List<?> identifiers = (functionScope != null) ? functionScope.getLocalVariableNames() : new ArrayList<>();
348+
List<FrameSlotKind> slotKinds = (functionScope != null) ? functionScope.getLocalVariableKinds() : new ArrayList<>();
349+
String functionFrameDescriptorName = name != null && !name.isEmpty() ? name : "<function>";
350+
FrameDescriptor descriptor = FrameSlotChangeMonitor.createFunctionFrameDescriptor(functionFrameDescriptorName, slotKinds, identifiers);
370351
FunctionDefinitionNode rootNode = FunctionDefinitionNode.create(language, source, descriptor, argSourceSections, saveArguments, body, formals, name, argPostProcess);
371352

372353
if (RContext.getInstance().getOption(ForceSources)) {
@@ -415,12 +396,11 @@ public RSyntaxNode specialLookup(SourceSection source, String symbol, boolean fu
415396
// function variables - they are mostly promises that has to be evaluated first anyway, and
416397
// the evaluation of the promise has to be done on slow-path.
417398
if (functionScope != null && !functionLookup) {
418-
if (functionScope.containsLocalVariable(symbol)) {
419-
LocalVariable localVar = functionScope.getLocalVariable(symbol);
420-
logger.fine(() -> "Creating ReadNode for local variable " + localVar);
421-
assert localVar != null;
422-
assert FrameIndex.isInitializedIndex(localVar.getFrameIndex());
423-
var readVariableNode = ReadVariableNode.createLocalVariableLookup(symbol, localVar.getFrameIndex());
399+
Integer locVarFrameIdx = functionScope.getLocalVariableFrameIndex(symbol);
400+
if (locVarFrameIdx != null) {
401+
logger.fine(() -> "Creating ReadNode for local variable " + symbol);
402+
assert FrameIndex.isInitializedIndex(locVarFrameIdx);
403+
var readVariableNode = ReadVariableNode.createLocalVariableLookup(symbol, locVarFrameIdx);
424404
return ReadVariableNode.wrap(source, readVariableNode);
425405
}
426406
}

‎com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/FrameIndexNode.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
import com.oracle.truffle.api.CompilerDirectives;
2828
import com.oracle.truffle.api.frame.Frame;
2929
import com.oracle.truffle.api.frame.FrameDescriptor;
30-
import com.oracle.truffle.api.frame.FrameSlotKind;
3130
import com.oracle.truffle.api.frame.FrameSlotTypeException;
3231
import com.oracle.truffle.api.nodes.InvalidAssumptionException;
3332
import com.oracle.truffle.api.nodes.NodeCost;
@@ -111,7 +110,7 @@ private FrameIndexNode resolveFrameIndex(Frame frame) {
111110

112111
private static final class PresentFrameIndexNode extends FrameIndexNode {
113112
private final ConditionProfile isNotNullProfile = ConditionProfile.createBinaryProfile();
114-
private final ConditionProfile isObjectProfile = ConditionProfile.createBinaryProfile();
113+
private final ConditionProfile isPrimitiveProfile = ConditionProfile.createBinaryProfile();
115114
private final ValueProfile frameTypeProfile = ValueProfile.createClassProfile();
116115
private final int frameIndex;
117116

@@ -127,8 +126,7 @@ public int executeFrameIndex(Frame frame) {
127126
@Override
128127
public boolean hasValue(Frame frame) {
129128
Frame typedFrame = frameTypeProfile.profile(frame);
130-
FrameSlotKind slotKind = FrameSlotChangeMonitor.getFrameSlotKindInFrame(typedFrame, frameIndex);
131-
if (!(isObjectProfile.profile(slotKind == FrameSlotKind.Object))) {
129+
if (isPrimitiveProfile.profile(FrameSlotChangeMonitor.isPrimitiveFrameSlotKind(typedFrame, frameIndex))) {
132130
// A primitive frame slot kind always has some value.
133131
return true;
134132
} else {

‎com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/function/WrapArgumentNode.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2013, 2020, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2013, 2022, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it

‎com.oracle.truffle.r.parser/src/com/oracle/truffle/r/parser/R.g4‎

Lines changed: 5 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,6 @@ import com.oracle.truffle.r.runtime.nodes.RCodeBuilder;
6161
import com.oracle.truffle.r.runtime.nodes.RCodeBuilder.Argument;
6262
import com.oracle.truffle.r.runtime.nodes.RCodeBuilder.RCodeToken;
6363
import com.oracle.truffle.r.runtime.parsermetadata.FunctionScope;
64-
import com.oracle.truffle.r.runtime.parsermetadata.LocalVariable;
6564
}
6665

6766
@rulecatch {
@@ -162,27 +161,9 @@ import com.oracle.truffle.r.runtime.parsermetadata.LocalVariable;
162161
private static void addArgumentAsLocalVariable(FunctionScope functionScope, String argIdentifier) {
163162
assert functionScope != null;
164163
assert argIdentifier != null;
165-
int nextFrameIndex = functionScope.getNextLocalVariableFrameIndex();
166-
var localVar = new LocalVariable(argIdentifier, FrameSlotKind.Illegal, nextFrameIndex);
167-
functionScope.addLocalVariable(localVar);
164+
functionScope.addLocalVariable(argIdentifier, FrameSlotKind.Illegal);
168165
}
169166

170-
/**
171-
* Helper function that creates a localVariable from assignment if lhs is a lookup.
172-
* Returns null otherwise.
173-
*/
174-
private LocalVariable localVariable(FunctionScope functionScope, RSyntaxNode lhs, RSyntaxNode rhs) {
175-
if (lhs instanceof RSyntaxLookup) {
176-
String identifier = ((RSyntaxLookup) lhs).getIdentifier();
177-
FrameSlotKind type = infereType(rhs);
178-
int localVarFrameIdx = functionScope.getNextLocalVariableFrameIndex();
179-
var localVar = new LocalVariable(identifier, type, localVarFrameIdx);
180-
return localVar;
181-
} else {
182-
return null;
183-
}
184-
}
185-
186167
private static FrameSlotKind infereType(RSyntaxNode rhs) {
187168
// TODO
188169
return FrameSlotKind.Illegal;
@@ -197,9 +178,10 @@ import com.oracle.truffle.r.runtime.parsermetadata.LocalVariable;
197178
*/
198179
private void maybeAddLocalVariable(FunctionScope functionScope, RSyntaxNode lhs, RSyntaxNode rhs) {
199180
if (functionScope != null) {
200-
LocalVariable localVariable = localVariable(functionScope, lhs, rhs);
201-
if (localVariable != null) {
202-
functionScope.addLocalVariable(localVariable);
181+
if (lhs instanceof RSyntaxLookup) {
182+
String identifier = ((RSyntaxLookup) lhs).getIdentifier();
183+
FrameSlotKind type = infereType(rhs);
184+
functionScope.addLocalVariable(identifier, type);
203185
}
204186
}
205187
}

‎com.oracle.truffle.r.parser/src/com/oracle/truffle/r/parser/RParser.java‎

Lines changed: 5 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,6 @@
6969
import com.oracle.truffle.r.runtime.nodes.RSyntaxLookup;
7070
import com.oracle.truffle.r.runtime.nodes.RSyntaxNode;
7171
import com.oracle.truffle.r.runtime.parsermetadata.FunctionScope;
72-
import com.oracle.truffle.r.runtime.parsermetadata.LocalVariable;
7372

7473
@SuppressWarnings("all")
7574
public class RParser extends Parser {
@@ -277,27 +276,9 @@ private RSyntaxNode operator(Token op) {
277276
private static void addArgumentAsLocalVariable(FunctionScope functionScope, String argIdentifier) {
278277
assert functionScope != null;
279278
assert argIdentifier != null;
280-
int nextFrameIndex = functionScope.getNextLocalVariableFrameIndex();
281-
var localVar = new LocalVariable(argIdentifier, FrameSlotKind.Illegal, nextFrameIndex);
282-
functionScope.addLocalVariable(localVar);
279+
functionScope.addLocalVariable(argIdentifier, FrameSlotKind.Illegal);
283280
}
284281

285-
/**
286-
* Helper function that creates a localVariable from assignment if lhs is a lookup.
287-
* Returns null otherwise.
288-
*/
289-
private LocalVariable localVariable(FunctionScope functionScope, RSyntaxNode lhs, RSyntaxNode rhs) {
290-
if (lhs instanceof RSyntaxLookup) {
291-
String identifier = ((RSyntaxLookup) lhs).getIdentifier();
292-
FrameSlotKind type = infereType(rhs);
293-
int localVarFrameIdx = functionScope.getNextLocalVariableFrameIndex();
294-
var localVar = new LocalVariable(identifier, type, localVarFrameIdx);
295-
return localVar;
296-
} else {
297-
return null;
298-
}
299-
}
300-
301282
private static FrameSlotKind infereType(RSyntaxNode rhs) {
302283
// TODO
303284
return FrameSlotKind.Illegal;
@@ -312,9 +293,10 @@ private static FrameSlotKind infereType(RSyntaxNode rhs) {
312293
*/
313294
private void maybeAddLocalVariable(FunctionScope functionScope, RSyntaxNode lhs, RSyntaxNode rhs) {
314295
if (functionScope != null) {
315-
LocalVariable localVariable = localVariable(functionScope, lhs, rhs);
316-
if (localVariable != null) {
317-
functionScope.addLocalVariable(localVariable);
296+
if (lhs instanceof RSyntaxLookup) {
297+
String identifier = ((RSyntaxLookup) lhs).getIdentifier();
298+
FrameSlotKind type = infereType(rhs);
299+
functionScope.addLocalVariable(identifier, type);
318300
}
319301
}
320302
}

‎com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/RRuntime.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ public static MaterializedFrame createNewFrame(FrameDescriptor frameDescriptor)
235235
* Create an {@link VirtualFrame} for a non-function environment, e.g., a package frame or the
236236
* global environment.
237237
*/
238-
public static MaterializedFrame createNonFunctionFrameNew(String name) {
238+
public static MaterializedFrame createNonFunctionFrame(String name) {
239239
FrameDescriptor frameDescriptor = FrameSlotChangeMonitor.createUninitializedFrameDescriptor(name);
240240
MaterializedFrame frame = Truffle.getRuntime().createMaterializedFrame(RArguments.createUnitialized(), frameDescriptor);
241241
FrameSlotChangeMonitor.initializeNonFunctionFrameDescriptor(frameDescriptor, frame);

‎com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/RSerialize.java‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,6 @@
110110
import com.oracle.truffle.r.runtime.nodes.RSyntaxNode;
111111
import com.oracle.truffle.r.runtime.nodes.RSyntaxVisitor;
112112
import com.oracle.truffle.r.runtime.parsermetadata.FunctionScope;
113-
import com.oracle.truffle.r.runtime.parsermetadata.LocalVariable;
114113

115114
// Code loosely transcribed from GnuR serialize.c.
116115

@@ -2919,8 +2918,7 @@ private static RSyntaxNode processCall(Object car, Object cdr, @SuppressWarnings
29192918
if (assignmentTarget.value instanceof RSyntaxLookup) {
29202919
// assignment into a variable
29212920
String identifier = ((RSyntaxLookup) assignmentTarget.value).getIdentifier();
2922-
int varFrameIndex = functionScope.getNextLocalVariableFrameIndex();
2923-
functionScope.addLocalVariable(new LocalVariable(identifier, FrameSlotKind.Illegal, varFrameIndex));
2921+
functionScope.addLocalVariable(identifier, FrameSlotKind.Illegal);
29242922
}
29252923
}
29262924
RSyntaxNode call = RContext.getASTBuilder().call(RSyntaxNode.LAZY_DEPARSE, lhs, arguments);
@@ -2954,8 +2952,7 @@ private static List<RCodeBuilder.Argument<RSyntaxNode>> processArguments(Object
29542952
}
29552953
if (inFunctionDeclaration) {
29562954
assert name != null;
2957-
var localVar = new LocalVariable(name, FrameSlotKind.Illegal, functionScope.getNextLocalVariableFrameIndex());
2958-
functionScope.addLocalVariable(localVar);
2955+
functionScope.addLocalVariable(name, FrameSlotKind.Illegal);
29592956
}
29602957
RSyntaxNode value = arglist.car() == RMissing.instance ? null : process(arglist.car(), false, index == 1 ? assignedName : null, functionScope);
29612958
list.add(RCodeBuilder.argument(RSyntaxNode.LAZY_DEPARSE, name, value));

‎com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/data/RDataFactory.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -679,7 +679,7 @@ public final RFunction createFunction(String name, String packageName, RootCallT
679679

680680
@TruffleBoundary
681681
public final REnvironment createInternalEnv() {
682-
return traceDataCreated(new REnvironment.NewEnv(RRuntime.createNonFunctionFrameNew("<internal-env-" + environmentCount.incrementAndGet() + ">"), REnvironment.UNNAMED));
682+
return traceDataCreated(new REnvironment.NewEnv(RRuntime.createNonFunctionFrame("<internal-env-" + environmentCount.incrementAndGet() + ">"), REnvironment.UNNAMED));
683683
}
684684

685685
@TruffleBoundary
@@ -689,12 +689,12 @@ public final REnvironment.NewEnv createNewEnv(FrameDescriptor desc, String name)
689689

690690
@TruffleBoundary
691691
public final REnvironment.NewEnv createNewEnv(String name) {
692-
return traceDataCreated(new REnvironment.NewEnv(RRuntime.createNonFunctionFrameNew("<new-env-" + environmentCount.incrementAndGet() + ">"), name));
692+
return traceDataCreated(new REnvironment.NewEnv(RRuntime.createNonFunctionFrame("<new-env-" + environmentCount.incrementAndGet() + ">"), name));
693693
}
694694

695695
@TruffleBoundary
696696
public final REnvironment createNewEnv(String name, boolean hashed, int initialSize) {
697-
REnvironment.NewEnv env = new REnvironment.NewEnv(RRuntime.createNonFunctionFrameNew("<new-env-" + environmentCount.incrementAndGet() + ">"), name);
697+
REnvironment.NewEnv env = new REnvironment.NewEnv(RRuntime.createNonFunctionFrame("<new-env-" + environmentCount.incrementAndGet() + ">"), name);
698698
env.setHashed(hashed);
699699
env.setInitialSize(initialSize);
700700
return traceDataCreated(env);
@@ -1315,7 +1315,7 @@ public static RFunction createFunction(String name, String packageName, RootCall
13151315

13161316
@TruffleBoundary
13171317
public static REnvironment createInternalEnv(String name) {
1318-
return traceDataCreated(new REnvironment.NewEnv(RRuntime.createNonFunctionFrameNew("<internal-env-" + environmentCount.incrementAndGet() + ">"), name));
1318+
return traceDataCreated(new REnvironment.NewEnv(RRuntime.createNonFunctionFrame("<internal-env-" + environmentCount.incrementAndGet() + ">"), name));
13191319
}
13201320

13211321
public static REnvironment createInternalEnv() {
@@ -1329,12 +1329,12 @@ public static REnvironment.NewEnv createNewEnv(FrameDescriptor desc, String name
13291329

13301330
@TruffleBoundary
13311331
public static REnvironment.NewEnv createNewEnv(String name) {
1332-
return traceDataCreated(new REnvironment.NewEnv(RRuntime.createNonFunctionFrameNew("<new-env-" + environmentCount.incrementAndGet() + ">"), name));
1332+
return traceDataCreated(new REnvironment.NewEnv(RRuntime.createNonFunctionFrame("<new-env-" + environmentCount.incrementAndGet() + ">"), name));
13331333
}
13341334

13351335
@TruffleBoundary
13361336
public static REnvironment createNewEnv(String name, boolean hashed, int initialSize) {
1337-
REnvironment.NewEnv env = new REnvironment.NewEnv(RRuntime.createNonFunctionFrameNew("<new-env-" + environmentCount.incrementAndGet() + ">"), name);
1337+
REnvironment.NewEnv env = new REnvironment.NewEnv(RRuntime.createNonFunctionFrame("<new-env-" + environmentCount.incrementAndGet() + ">"), name);
13381338
env.setHashed(hashed);
13391339
env.setInitialSize(initialSize);
13401340
return traceDataCreated(env);

0 commit comments

Comments
 (0)