update test case

add ; REQUIRES: asserts for test case

Add more comments and rebase to up-to-date.

I'd like to upstream it first, and interface modification would be done in follow-up patches.

Rebase it and rerun bmk 2017 to make last check before launch.

We can land it firstly and update the interface to use MVT as simplest type to getRegisterClassForType parameter and call TLI->getTypeLegalizationCost before.
This can apply to where GetRegUsage works

update the comment of getRegisterClassForType. Make it landed firstly and update the interface to leverage MVT in follow-up work.

Gentle pin...

Gentle pin...

Add some comments at function interface prototype.

Change the interface prototype of getRegisterClassForType and make second parameter with default value.

Address comments to simplify the default implementation for targets.

Address comments. Add 3 function interfaces.

In D67148#1665626, @hfinkel wrote:

In D67148#1665615, @wuzish wrote:

In D67148#1665594, @hfinkel wrote:

Thanks for looking at this (it's been a problem for a long time). Let me suggest a different interface, which I believe will improve generality and reduce code duplication in the register-pressure estimator, and let me know what you think...

// Return the number of registers in the target-provided register class.
unsigned getNumberOfRegisters(unsigned ClassID = 0) const;

// Return the target-provided register class for the provided type.
unsigned getRegisterClassForType(Type *Ty) const;

The idea, then, is that we just calculate register usage for each register class separately (i.e., keep a hash table), and then when computing the interleaving factor, etc. we just iterate over all of the register classes returned by the target, and pick the smallest interleaving factor calculated over all of the register classes. There's probably even a nice way to construct a default implementation of this in the backend (although that we'd save for follow-up work).

Yes. Using ClassID is more general and fine-grained. But I think there is no need to iterate all kinds of register class because target register class is flexible and many register classes target provided are only different width with same position(such as gprc and g8rc), which makes too many kinds of register classes to maintain and some are just the same thing. It's not just use smallest interleaving factor calculated over all of the register classes and we also need to care about the overlapping relationship between different register class.

In all, it would be too fine-grained and little over-design, many register classes are just same behavior as separate with scalar and vector, int and float. What's your opinion?

I think that you misunderstood my suggestion. I did not mean that the backend register classes would be directly mapped into the classes returned by the TTI interface. These would, especially for non-trivial architectures, be abstracted for the purpose of the TTI interface. For PPC, Altivec, we'd have three class IDs for now (scalar float, scalar int, vectors), all with 32 registers. For VSX, we'd have two register class IDs, one for scalar ints (with 32 registers), and one for everything else (with 64 registers). (*) These don't need to have anything to do with the register classes actually defined in the backend. Do we need to capture finer-grained details than that in the heuristic (e.g. is your overlap suggestion capturing more than this)?

(*) Actually, for both cases, we can also have a separate class ID for scalar i1 types (with 8 registers).

In D67148#1665594, @hfinkel wrote:

Thanks for looking at this (it's been a problem for a long time). Let me suggest a different interface, which I believe will improve generality and reduce code duplication in the register-pressure estimator, and let me know what you think...

// Return the number of registers in the target-provided register class.
unsigned getNumberOfRegisters(unsigned ClassID = 0) const;

// Return the target-provided register class for the provided type.
unsigned getRegisterClassForType(Type *Ty) const;

The idea, then, is that we just calculate register usage for each register class separately (i.e., keep a hash table), and then when computing the interleaving factor, etc. we just iterate over all of the register classes returned by the target, and pick the smallest interleaving factor calculated over all of the register classes. There's probably even a nice way to construct a default implementation of this in the backend (although that we'd save for follow-up work).

Gentle pin..

gentle pin...

Could anybody run some bmk to see the performance result of AARCH64 and X86?
I have run on PowerPC, it has good influence in some bmks of spec2017 and no obvious degression.

Enable it for AARCH64 and x86, and address testcases.

This is a WIP draft and I have not fixed all check-all cases now. I want to get some quick feedback whether it's on the right direction and whether its performance is good at all different target. If you have time, could you please review it or give some comments, and help me to verify the performance at other targets?

Is there any fix patch proposed to track/fix the regression? @dnsampaio

In D32201#1625379, @wmi wrote:

In D32201#1620644, @wuzish wrote:

Should this good patch be rebased and continued to move on? @wmi
it'd be better to move on this work because D27366 is abandoned and D34608 is not updated for a long time, and those 3 patches are all trying to fix same issue and not landed successfully.
It would be great If it can land.

Thanks for bringing it to attention again.

I still think the patch will be very helpful for some cases especially when dynamic profile information is available (when FDO/SampleFDO is used). It is tricky if there is only static profile available. I will reevaluate the patch and see how it works if I change the patch to be functioning only when we have dynamic profile. If you are more interesting on the case when dynamic profile is not available -- i.e., try to make the patch more general, feel free to take over it and I will be happy to join discussion and review.

In D52709#1263679, @craig.topper wrote:

In D52709#1263663, @xbolva00 wrote:

Wouldn't it better to pull it out of instcombine into.. let say.. CodeSinkingPass?

Absolutely. This code doesn't belong in InstCombine. This code isn't even the code we would use if we had a real sinking pass. This is a very simple sinking algorithm. It only moves code to an immediate successor and it has a bogus one use check in it. I think we should have a real sinking pass. lib/Transforms/Scalar/Sink.cpp is probably a good starting point, I believe its only used by one target today. I think adding a disable here is a good first step towards being able to experiment with a real sinking pass.

Should this good patch be rebased and continued to move on? @wmi
it'd be better to move on this work because D27366 is abandoned and D34608 is not updated for a long time, and those 3 patches are all trying to fix same issue and not landed successfully.
It would be great If it can land.

Should this be rebased and continued to move on? Or it already could be addressed by https://reviews.llvm.org/D41765? @junbuml @qcolombet

Because we have PPCVSXSwapRemoval pass to hack the element order before P9 with vsx, the element order is not always standard normal order in register.
And the optimization of this patch will be conflict with PPCVSXSwapRemoval so that we can not get correct result during the process. The fix way is to not do this optmization before P9.

update patch due to find issue.

Sorry to forget to add revision address in commit. Close it manually.

Commit testcase firstly and address comments.

Gentle pin..

Gentle pin..

After run bmk 2017 cpu for fortran and c/c++, there is some noise, no obvious degression. Some have little improvement about 1%~2%.

Address comments

gentle pin... It's a follow-up of https://reviews.llvm.org/D60601

update testcases.

In D55506#1560499, @uweigand wrote:

In D55506#1560331, @wuzish wrote:

I have another question about any_*.

For example, I see you use any_fadd to match both fadd and strict_fadd. I guess ZSystem target is similar to POWER that there is no fp addition machine instruction with nearest rounding mode and no exception statically.
But the semantic of fadd node requires. So theoretically, fadd node should be mapped into 2 instructions for correct compiling. One sets rounding mode to nearest and clear exception handler bit(no exception happens), the other is float addition.
Do we not handle fadd like above because there is no function error for such fine-grained instruction semantic mostly? And avoid performance penalty? @uweigand

I believe you may be misunderstanding the semantics of the non-strict operations. These do *not* instruct codegen to actively generate code to switch rounding modes and/or clear exception bits. Rather, the use of a non-strict operation like fadd is an *assertion* that tells codegen that it may *assume* that the rounding mode is set to default, trapping exceptions are switched off, and exceptions flags are don't care (i.e. subsequent code will not check them). If you ever use fadd in a context where this assumption is not true, the behaviour is undefined. This means it is perfectly correct to implement fadd using the "normal" floating-point instruction.

Note that similarly, if a strict operation with a given rounding mode flag is used, this likely does *not* instruct coegen to actively generate code to implement this particular rounding mode. Rather, this is again simply an *assertion* that tells codegen that it may *assume* the current rounding mode is set to this specific value. Again, this means that it is perfectly correct to implement such an operation using the "normal" floating-point instruction that will use the current rounding mode.