On 11/13/19 4:43 PM, Michael Matz wrote:
Hi,
On Wed, 13 Nov 2019, Martin Liška wrote:
Not a review, just a few questions:
Hello.
Thank you for it.
1) what does it do if __builtin_expect* has been used, does it preserve
the probabilities and if in the end decides to expand as ifs, are those
probabilities retained through it?
No, it's currently not supported. I can consider adding that as a follow up.
But given that you apply the transformation even to small if ladders this
looses quite some info then.
Yes. Based on my experiments, the patch does not convert if-else chain
with __builtin_expect* right now.
2) for the reassoc-*.c testcases, do you get identical or better code
with the patch?
The code is the following:
Before:
Optimizing range tests a_5(D) -[10, 10] and -[26, 26]
into (a_5(D) & -17) != 10
<bb 2> [local count: 1073741823]:
_11 = a_5(D) & -17;
_12 = _11 == 10;
_1 = a_5(D) == 10;
_2 = a_5(D) == 12;
_10 = a_5(D) == 26;
_9 = _2 | _12;
if (_9 != 0)
goto <bb 4>; [56.44%]
else
goto <bb 3>; [43.56%]
After:
<bb 2> [local count: 1073741824]:
switch (a_2(D)) <default: <L5> [50.00%], case 10: <L6> [50.00%], case 12:
<L6> [50.00%], case 26: <L6> [50.00%]>
And here I bet that Jakub was asking for final code, not intermediate
code. In particular the bit trick with transforming a test for
a \in {10,26} into (a&-17)==10. The switch still tests for 10,26, and in
the end it should be ensured that the same trickery is employed to
actually implement the switch.
I agree that (a&-17)==10 is more elegant trick here ...
As seen, reassoc can generate a different range check and then it's about cmp1
| cmp2 | ..
I bet the explicit gswitch is quite equal representation.
As long as the final code is the same or better, sure. Otherwise it's a
more canonical representation with suboptimal expansion, and the latter
should be fixed otherwise it's introducing a regression.
... and yes, the gswitch expansion is not capable of such smart interval tests.
We can improve that in the future.
Martin
Ciao,
Michael.
