rakudrama
commented
2 years ago I have been looking into this.
We can compensate for the too-late inlining of _ImmutableList.length in constant propagation:
void ConstantPropagator::VisitStaticCall(StaticCallInstr* instr) {
...
case MethodRecognizer::kImmutableArrayLength:
case MethodRecognizer::kObjectArrayLength: {
ASSERT(instr->FirstArgIndex() == 0);
const Object& o = instr->ArgumentAt(0)->constant_value();
if (o.IsArray()) {
const auto& array = Array::Cast(o);
SetValue(instr, Integer::ZoneHandle(Z, Integer::New(array.Length())));
return;
}
break;
}
...It is slightly unsatisfactory to have another place that knows how to reduce a load of the length of a fixed-length list.
The GenericCheckBound is blocking further constant-folding because of this:
void ConstantPropagator::VisitGenericCheckBound(GenericCheckBoundInstr* instr) {
// Don't propagate constants through check, since it would eliminate
// the data dependence between the bound check and the load/store.
// Graph finalization will expose the constant eventually.
SetValue(instr, non_constant_);
}I think we can have our cake and eat it too. We don't have to eliminate the data dependence. We can propagate the constant index, but not replace the GenericCheckBoound with a constant.
This would enable the subsequent LoadIndexed to be constant-folded, but other uses would still be guarded.
Question: Is this right?
An amusing aside: I tried replacing, in ConstantPropagator::LoadIndexed, the index ->definition()->constant_value() with ->definition->OriginalDefinition()->constant_value(). This broke the dataflow, since the transfer function for an instruction was no longer a function of the inputs of the instruction. Lucky for me this broke the build.
Another approach is to add constant-folding of LoadIndexed to canonicalization, where there is no dataflow invariants to uphold, and it is reasonable to inspect OriginalDefinitions.
The JIT reduces some calls to dart2js's
CallStructurefactory constructor to a constant. AOT fails to do this.It appears that the reason is that not enough good things happen during canonicalization to make the inlinee look appealing:
_ImmutableList.lengthis not replaced withLoadFieldearly enough to allow constant-folding to simplify the inlinee . Curiously,[]is replaced with a bounds check and indexed load, but...GenericCheckBoundhas no constant-inputs canonicalization, or perhaps it can't be removed but inhibits other reductions.Static calls to the getters of fields of user-defined constants are replaced by
LoadFieldinstructions earlier, making them available to reduction by canonicalization or constant-folding. This leaves constant lists at a disadvantage when compared to user-defined constants.(I believe the JIT succeeds simply because it is happy to inline larger methods and does the constant-folding later.)
The factory constructor does a lookup in a const table. An example of a call that the JIT inlines and reduces to a constant is here.
Consider this simplified version, using a single level of array:
When the inlining decision is made,
_common.lengthis still a static call. It has not been reduced to4, so the path with the constructor call is still in the callee graph:Interestingly, I can write a list wrapper :
This version gets inlined, and the final code for
demo1contains a load of the constant value:The two-level version still does not work. This is because the
LoadIndexedis not reduced to a constant. I suspect what is blocking this is thatGenericCheckBoundhas a no-opCanonicalizemethod, or is opaque to theLoadIndexedcanonicalization.This is a full stand-alone test,
demo1should have the inlined constant:Full code for simplified version:
Simplified version with wrapper: