Remove unnecessary idiom recognition assertions #17949

jdmpapin · 2023-08-14T17:36:56Z

There are uses of TR_ASSERT() in idiom recognition that check for cases that are not necessarily impossible and that are handled conservatively in builds where TR_ASSERT() is not checked. Assertions are not a good way to detect and report these cases, since assertion failure is not likely to be due to a bug.

This commit removes a number of these assertions. Now the conservative logic will apply in all builds. When it does, a static debug counter will be incremented, and (when tracing idiom recognition) a message will be printed to the log.

Anyone who wants to look for transformations that have been prevented due to incomplete support for a particular case can use the new static debug counters.

Additionally, use dumpOptDetails() to trace when a transformer fails. In that case it's misleading to leave the log showing only the message from performTransformation().

Fixes #17819

hzongaro

I think your change looks good - just one minor question/comment.

I notice there are at least four other places in Idiom Recognition that have a TR_ASSERT that says "not implemented yet". As the goal in the short-term is to get PROD_WITH_ASSUMES working for sanity.functional test runs, fixing just this one is fine for now, but may I ask you to open a follow up issue to track cleaning up the remaining four - assuming that they also describe situations that can be safely ignored?

runtime/compiler/optimizer/IdiomRecognition.cpp

jdmpapin · 2023-08-15T20:14:50Z

I'll generalize this PR at least a bit

There are uses of TR_ASSERT() in idiom recognition that check for cases that are not necessarily impossible and that are handled conservatively in builds where TR_ASSERT() is not checked. Assertions are not a good way to detect and report these cases, since assertion failure is not likely to be due to a bug. This commit removes a number of these assertions. Now the conservative logic will apply in all builds. When it does, a static debug counter will be incremented, and (when tracing idiom recognition) a message will be printed to the log. Anyone who wants to look for transformations that have been prevented due to incomplete support for a particular case can use the new static debug counters. Additionally, use dumpOptDetails() to trace when a transformer fails. In that case it's misleading to leave the log showing only the message from performTransformation().

jdmpapin · 2023-08-16T20:58:12Z

Updated. Along with some assertions in the transformations, I caught analyzeByteBoolTable() and analyzeCharBoolTable(). What were the other two places you had in mind?

hzongaro · 2023-08-17T17:53:14Z

What were the other two places you had in mind?

I think I miscounted - the third was in analyzeBoolTable, which was the one you originally changed. The fourth is in TR_CISCTransformer::modifyBlockByVersioningCheck, and so far as I can tell, that method is never actually used. Looking at it more closely, I think it represents a real assertion, rather than simply flagging something that was unimplemented.

Thanks for catching the additional changes in the transformations! I'll review your changes.

hzongaro

Looks good. Thanks!

hzongaro · 2023-08-17T20:00:11Z

Jenkins test sanity all jdk11,jdk17

hzongaro · 2023-08-18T12:02:47Z