C++: Speed up boolConversionLowerBound (and boolConversionUpperBound).

[geoffw0]

trafficstars

Possible improvement to SimpleRangeAnalysis::boolConversionLowerBound, motivated by a recent regression in the DCA tuple sums reports for this predicate (though I think it regressed at least one more time before this) - https://github.com/github/codeql-dca-main/issues/5626):

kamailio__kamailio	cpp/unsigned-difference-expression-compared-zero:SimpleRangeAnalysis::boolConversionLowerBound#a97350a1#ff#40d9bkw9dl49ghmceol5t3dj26s# (standard)	1	1	❌	13030410	0	17596713	0	4566303	0.35

The change takes some repeat calculations out into separate predicates, with the aim of discouraging repeat calculations and encouraging exprIsUsedAsBool to be joined with getTruncated*Bounds early. Following this change, when I run cpp/unsigned-difference-expression-compared-zero locally on kamailio__kamailio I no longer see boolConversionLowerBound in the most expensive predicates report (nor do I see either of the two new predicates).

DCA run of this change here:

• I think we can ignore the failures in this DCA run, these are projects we've been having problems with in recent nightly runs.
• sadly the 'Tuple sums, per source and predicate' table is truncated (and the 'complete table' link doesn't work for me), but under 'Tuple sums, per source' we can see an improvement:

kamailio__kamailio	1	1	✔	✗	42276185596	0	42068619900	0	-207565696	-0.00491

@rdmarsh2 - mentioned to keep you in the loop as I believe you are currently working on SimpleRangeAnalysis.

@jketema - mentioned as I know you're working on some other DCA regressions / issues at the moment.

[jketema]

https://github.com/github/codeql/pull/9872 might have flipped this back to not being a regression. Might still be good to rewrite this to make it more stable in the long run.

I haven't looked at tuple counts (yet), but I wonder if pushing down exprIsUsedAsBool(expr) is the right approach. What happens if we completely get rid of all exprIsUsedAsBool(expr) in boolConversionLowerBound and define a predicate that does boolConversionLowerBound(expr) and exprIsUsedAsBool(expr) (maybe with some pragma to make sure things happen in the oder we want).

[geoffw0]

@jketema this is an old PR, I'd like to either minimally update it and get it merged, or else close it this week. Do you have any idea if it's still relevant?

[MathiasVP]

Sadly, the DCA data about the actual evaluation time changes has been deleted. I'd be happy to review it if you merge in the latest main and rerun DCA. The QL changes certainly looks sensible to me, at least.

[jketema]

@jketema this is an old PR, I'd like to either minimally update it and get it merged, or else close it this week. Do you have any idea if it's still relevant?

As far as I recall, the tuple counts on kamailio were not very stable at the time. I'm not sure if that has changed, as I haven't looked at any nightly DCA runs recently. I propose we just drop this, as it's not likely going to give us a large performance advantage. If you disagree, then please do as @MathiasVP suggests.

[geoffw0]

Closing.

If these predicates are really a big problem I'm sure we'll encounter them again (if we haven't already).