Omr: Assert failure in JitBuilder: Cannot branch or fall through

[Leonardo2718]

The new JitBuilder optimization strategy, recently added in #839, can causes the following assert:

Assertion failed at ../compiler/optimizer/VPHandlers.cpp:10042: !cannotFallThrough
    Cannot branch or fall through
compiling @factorial.lua:3:@factorial.lua:3_9 at level: warm
#0: function TR_LinuxCallStackIterator::printStackBacktrace(TR::Compilation*)+0x26 [0x89a516]
#1: function TR_Debug::printStackBacktrace()+0x1d [0x5f227d]
#2: luav() [0x5548fd]
#3: function TR::assertion(char const*, int, char const*, char const*, ...)+0x80 [0x554b50]
#4: luav() [0x7a4c1f]
#5: function TR::ValuePropagation::launchNode(TR::Node*, TR::Node*, int)+0xa1 [0x6f8d81]
#6: function TR::ValuePropagation::processTrees(TR::TreeTop*, TR::TreeTop*)+0x27f [0x6f91ef]
#7: function TR::GlobalValuePropagation::processBlock(TR_StructureSubGraphNode*, bool, bool)+0x31e [0x6e61ce]
#8: function TR::GlobalValuePropagation::processRegionNode(TR_StructureSubGraphNode*, bool, bool)+0x404 [0x6eb4e4]
#9: function TR::GlobalValuePropagation::processRegionNode(TR_StructureSubGraphNode*, bool, bool)+0x9b [0x6eb17b]
#10: function TR::GlobalValuePropagation::processRegionNode(TR_StructureSubGraphNode*, bool, bool)+0x9b [0x6eb17b]
#11: function TR::GlobalValuePropagation::processRegionNode(TR_StructureSubGraphNode*, bool, bool)+0x9b [0x6eb17b]
#12: function TR::GlobalValuePropagation::processRegionNode(TR_StructureSubGraphNode*, bool, bool)+0x9b [0x6eb17b]
#13: function TR::GlobalValuePropagation::processRegionNode(TR_StructureSubGraphNode*, bool, bool)+0x9b [0x6eb17b]
#14: function TR::GlobalValuePropagation::processRegionNode(TR_StructureSubGraphNode*, bool, bool)+0x9b [0x6eb17b]
#15: function TR::GlobalValuePropagation::processRegionNode(TR_StructureSubGraphNode*, bool, bool)+0x9b [0x6eb17b]
#16: function TR::GlobalValuePropagation::processRegionNode(TR_StructureSubGraphNode*, bool, bool)+0x9b [0x6eb17b]
#17: function TR::GlobalValuePropagation::processRegionNode(TR_StructureSubGraphNode*, bool, bool)+0x9b [0x6eb17b]
#18: function TR::GlobalValuePropagation::processRegionSubgraph(TR_StructureSubGraphNode*, bool, bool, bool)+0xf6 [0x6eb766]
#19: function TR::GlobalValuePropagation::processAcyclicRegion(TR_StructureSubGraphNode*, bool, bool)+0x34 [0x6eb864]
#20: function TR::GlobalValuePropagation::determineConstraints()+0xb1 [0x6ebe81]
#21: function TR::GlobalValuePropagation::perform()+0x2f5 [0x6ed235]
#22: function OMR::Optimizer::performOptimization(OptimizationStrategy const*, int, int, int)+0xb23 [0x64dab3]
#23: function OMR::Optimizer::optimize()+0x2ea [0x64f64a]
#24: function OMR::Compilation::performOptimizations()+0x9d [0x5d6e7d]
#25: function OMR::Compilation::compile()+0x81c [0x5dde7c]
#26: function compileMethodFromDetails(OMR_VMThread*, TR::IlGeneratorMethodDetails&, TR_Hotness, int&)+0x309 [0x7e8bb9]
#27: function compileMethodBuilder+0x6a [0x54b70a]
#28: function luaJ_invokejit+0x47 [0x5343c7]
#29: function luaJ_compile+0x9 [0x534409]

[0xdaryl]

Can you provide instructions for someone to investigate this problem? i.e., how to get and run your Lua test case in a manner to reproduce this problem?

[Leonardo2718]

After spending some time investigating this issue I have been able to create a test case to reproduce this behavior. The code can be seen on the vpassert of my personal fork.

Executing the sample I added (on my x86-64 linux machine) using a debug build of JitBuilder results in the following failure:

Assertion failed at ../compiler/optimizer/VPHandlers.cpp:10042: !cannotFallThrough
    Cannot branch or fall through
compiling src/vpassert.cpp:48:join at level: warm
#0: ./vpassert() [0x79b056]
#1: ./vpassert() [0x4db7c3]
#2: ./vpassert() [0x42c2d5]
#3: ./vpassert() [0x42c516]
#4: ./vpassert() [0x69d086]
#5: ./vpassert() [0x69d3b8]
#6: ./vpassert() [0x5de74a]
#7: ./vpassert() [0x5d7704]
#8: ./vpassert() [0x5c1ddb]
#9: ./vpassert() [0x5c04a0]
#10: ./vpassert() [0x5c1784]
#11: ./vpassert() [0x5c13be]
#12: ./vpassert() [0x5c051b]
#13: ./vpassert() [0x5c0435]
#14: ./vpassert() [0x5c030c]
#15: ./vpassert() [0x5bf542]
#16: ./vpassert() [0x5241b8]
#17: ./vpassert() [0x5210ea]
#18: ./vpassert() [0x4a7c5b]
#19: ./vpassert() [0x4a7592]
#20: ./vpassert() [0x6d89d3]
#21: ./vpassert() [0x41c763]
#22: ./vpassert() [0x402946]
#23: function __libc_start_main+0xf1 [0x7f3ac68dd511]
#24: ./vpassert() [0x402a5a]

vpassert: ../compiler/infra/Assert.cpp:140: void assumeDontCallMeDirectlyImpl(const char*, int32_t, const char*, const char*, __va_list_tag*, bool): Assertion `0' failed.
zsh: abort (core dumped)  TR_Options=paranoidoptcheck,tracefull,log=trace.log ./vpassert

Looking at the trees (after a pass of Tree Simplification):

n1n       BBStart <block_2>
n8n       istore  _T3  <auto slot 3>[#616  _T3  ]
n5n         iconst 0
n14n      astore  _T4  <auto slot 4>[#617  _T4  ]
n11n        aload  elem1<parm 0 LElement;>[#614  Parm]
n19n      astore  _T5  <auto slot 5>[#619  _T5  ]
n16n        aloadi  unknown field[#618  Shadow]
n15n          aload  _T4  <auto slot 4>[#617  _T4  ]
n31n      istore  _T6  <auto slot 6>[#620  _T6  ]
n28n        acmpeq
n27n          iload  _T3  <auto slot 3>[#616  _T3  ]
n26n          aload  _T5  <auto slot 5>[#619  _T5  ]
n41n      istore  _T7  <auto slot 7>[#621  _T7  ]
n38n        iconst 0
n46n      ificmpeq --> block_13 BBStart at n36n ()
n44n        iload  _T6  <auto slot 6>[#620  _T6  ]
n45n        iload  _T7  <auto slot 7>[#621  _T7  ]
n43n      BBEnd </block_2> =====

n47n      BBStart <block_16>
n56n      astore  _T8  <auto slot 8>[#622  _T8  ]
n53n        aload  elem2<parm 1 LElement;>[#615  Parm]
n62n      astore  <temp slot 9>[#623  _T9  ]
n59n        aload  elem1<parm 0 LElement;>[#614  Parm]
n65n      astorei  unknown field[#618  Shadow]
n64n        aload  <temp slot 9>[#623  _T9  ]
n63n        aload  _T8  <auto slot 8>[#622  _T8  ]
n50n      BBEnd </block_16> =====

n36n      BBStart <block_13>
n68n      return
n67n      BBEnd </block_13>

the root cause of the problem is that the acmpeq node (n28n) has an iload as one of the operands (n27n), which returns an integer instead of an address. This type inconsistency makes the IL ill-formed.

[mstoodle]

Thanks for tracking down the reason for the assert, @Leonardo2718 .

I agree that the IlBuilder services should really do a stronger job of checking for consistent data types in operands and either asserting or automatically converting types where an obvious correct thing to do exists.

[mstoodle]

@Leonardo2718 I believe you can "work around" this issue for the moment (by providing consistent types to JitBuilder on your side), so the focus for this issue should be about fixing the strategic problem with enforcing/managing type consistency in JitBuilder & compiler, as opposed to helping you move forward.

Is that a reasonable summary?

[Leonardo2718]

@mstoodle yes, I think that's a good summary.

I do in fact work around the problem in my own work. This was the reason I created the smaller test case. I wanted to have some code we can test any eventual JitBuilder solution against that doesn't rely on my project.

[charliegracie]

I believe this has been resolved. Am I correct?

[Leonardo2718]

Actually, I think the underlying issue still exists.

Although I am able to work around the issue (and avoid the assert failure), the real problem is that JitBuilder is allowing type inconsistent IL to be generated. This is a serious usability problem as it takes a good understanding of Testarossa IL to see the real issue. This precisely the kind of thing JitBuilder is supposed to help with.

Perhaps this issue should be closed and a new issue opened that addresses this problem more clearly/directly? @mstoodle @charliegracie @0xdaryl thoughts?

[0xdaryl]

I think the revitalized ILVerifier should be able to catch and assert on problems like this. I think the "workaround" addresses this particular issue in JitBuilder at its source and hence can be closed assuming it was merged (was it, because it didn't reference this issue if it was?).

Are there other known examples of JitBuilder creating type inconsistent trees? If not, I'd rather not create a new issue to track potential problems only, but instead use the ILVerifier to find inconsistencies in the trees and create issues as they come up.

[Leonardo2718]

I think the "workaround" addresses this particular issue in JitBuilder at its source and hence can be closed assuming it was merged (was it, because it didn't reference this issue if it was?).

Actually, the work around was in a downstream project that consumes JitBuilder.

However, the ILValidator will indeed catch/report these kinds of problems now.

1494 made it so IL is validated immediately after IL generation in IlInjector. This effectively ensures that IL generated by JitBuilder will be validated before any optimizations are performed.

I think this is an effective solution for this issue so I will close it now.