Fix X86 MulDecomposer performance

[zl-wang]

We ran into performance issues when a multiply(by a constant) operation is decomposed into 5 simple operations. On nearly all modern X86 implementations (in the last decade at least), integer multiply instruction has a latency of 3 cycles. Assuming lea/add/sub/shl simple operations having a single cycle latency, you can see why the performance degraded when it is decomposed (usually the sequence has back to back register dependency). Here is a quick fix by conditioning the decomposition on the number of simple operations (although preferably this should be done according to processor specifics).

Also, fix two locations where two ADD(s) are most likely inferior to a single shift.

[zl-wang]

@vijaysun-omr please review/test/merge. once that is done, i will backport it to 0.57 repo.

[vijaysun-omr]

I will request a review from @0xdaryl on this, even as I do the testing based on looking at the (relatively safe) changes

[vijaysun-omr]

Jenkins build all

[vijaysun-omr]

Maybe this "3" should be named via an macro constant, so it appears to be less "magic".

Having the ability to change this value via an env var to experiment would also be handy

[zl-wang]

@0xdaryl there might be some historical background re X86 codegen doing multiply-by-4 through add twice. in general, add twice is slower than a single shift-left-by-2. please provide the historical reason of doing that and whether it is still relevant, before i make these "corrections".

[vijaysun-omr]

I measured this on multiplication by 52 which is what I saw in one of our benchmarks. While performance was much better than the original sequence, it was still slower by 1-3% it looked like, vs the performance with the env var set to disable all this logic and just just an x86 mul instruction. Maybe we should be using the decomposer logic only for constants that would need less than 3 instructions ? @zl-wang and @0xdaryl opinions are needed for choosing the default cutoff for the number of instructions in the end.

[zl-wang]

Two possible approaches going forward:

1. simpler: eliminate all cases where 3-instr sequence is used;
2. complex: need detailed knowledge of instr issuing on x86 implementations, how many each type of unit are available in the core, and the specific 3-instr sequence has the potential to be issued in parallel concurrently. then, keep those 3-instr cases (expecting to outperform mul op itself most likely).

[0xdaryl]

there might be some historical background re X86 codegen doing multiply-by-4 through add twice. in general, add twice is slower than a single shift-left-by-2

This code really needs to be looked at through a lens from 2003, when it was first tuned and contributed and then largely left alone for the past 20 years. In the Pentium 4 days, immediate shifts were more expensive than adds. I looked back at some historical documentation and found that SHL by a constant had a latency of 4-5 cycles, but a single ADD register is only a 0.5-1 .5 cycles (therefore two adds are cheaper than a single constant shift). This might explain some of the tuning decisions here.

I think a handful of people may have poked at bits of this code over the years, but a comprehensive review of the tuning decisions for modern x86 processors wasn't done and seems warranted.

[vijaysun-omr]

I suggest a staged approach where

simpler: eliminate all cases where 3-instr sequence is used;

is done first. @zl-wang could you please make this change and ready this PR for delivery ?

[zl-wang]

sure ... let me get onto it today or tomorrow

[zl-wang]

@r30shah @knn-k codegen modernization re mulDecompose might be worth of another look. i will review p codegen soon in this regard.

[zl-wang]

I used the C program below and gcc -O3 to verify our mul decomposition strategy: (compare what is generated for each mul operation)

void test(int *array, int x)
{
	array[0] = x*3;
	array[1] = (x+1)*5;
	array[2] = (x+2)*6;
	array[3] = (x+3)*9;
	array[4] = (x+4)*10;
	array[5] = (x+5)*11;
	array[6] = (x+6)*12;
	array[7] = (x+7)*13;
	array[8] = (x+8)*7;
	array[9] = (x+9)*14;
	array[10] = (x+10)*15;
	array[11] = (x+11)*17;
	array[12] = (x+12)*18;
	array[13] = (x+13)*19;
	array[14] = (x+14)*20;
	array[15] = (x+15)*21;
	array[16] = (x+16)*22;
	array[17] = (x+17)*23;
	array[18] = (x+18)*24;
	array[19] = (x+19)*25;
	array[20] = (x+20)*26;
	array[21] = (x+21)*27;
	array[22] = (x+22)*28;
	array[23] = (x+23)*29;
	array[24] = (x+24)*30;
	array[25] = (x+25)*31;
}

a few observations:

1. it is generally true that gcc cut off decomposition at 3 instructions (imul instruction is used in those cases directly);
2. in all cases, we are doing equally well or slightly better (for *11, *15, *19 cases. gcc chose to have a reg mov -- resulting in 3 instr in total, whereas we remain 2).

[vijaysun-omr]

Thanks, the current implementation does quite well in my runs on some relevant benchmarks. I have asked for further confirmation using the state of this PR as it stands. If that also checks out, I feel it should be ready for a final review from @0xdaryl

[0xdaryl]

Jenkins build all

[vijaysun-omr]

@0xdaryl is this ready for merging ? Please go ahead when you are ready

[0xdaryl]

Other than the comment removal, I think this is fine.

[0xdaryl]

Can you remove the commentary and the commented-out code? As mentioned in an earlier PR comment, this was a point-in-time decision based on the technology at the time that people have tinkered with over the years for various hardware evolutions.

I think it is fair to remove it and just use your approach by default.

[zl-wang]

done

[0xdaryl]

Jenkins build xlinux,win,osx