Thanks for your review @MaskRay
Also thanks Hubert for your good comments. @hubert.reinterpretcast

That is quite interesting, why they designed the feature to work that way. Is it recommended to reference debug sections through the label minus the length field size (4 or 12) or they provide some means to simplify the calculation?

In D94668#2500530, @echristo wrote:

I'm not quite sure I understand. What's the issue between xcoff and emission here? Does it not support subtraction? It's been a while and I can't recall.

-eric

In D94670#2500531, @echristo wrote:

Can you provide a little more detail on the motivation here? Thanks!

-eric

In D94668#2497861, @nemanjai wrote:

Added Eric as the owner of debug info.

fix according to comments:
1: unchanged the Offset type
2: fix Lint warnings

gentle ping

gentle ping

Appreciate your review @jsji . Could you please help to have a look at this patch's patent https://reviews.llvm.org/D92069. Thanks again.

1: set the register pressure factor to 1.5

In D86864#2472470, @steven.zhang wrote:

Is it better to have the check before 'if (PDT->dominates(To, BB)) {' ?

In D86864#2472387, @LiuChen3 wrote:

In D86864#2472375, @shchenz wrote:

In D86864#2472369, @LiuChen3 wrote:

I can surely do that. But I think the most reasonable solution would be fix the compiling time issue. Since compiling time tests I did before does not expose any regression, your test case must be a little special. Could you find out the special point, for example the function has too many blocks or some/many blocks in the function has too many instructions? Thanks.

I think the increase in compile time is because the function has too many instructions and blocks. The function has Thousands of lines of instructions. Can you add limitation for the number of instructions or the number of blocks so the check for 'store' can end early?

Could you please help to check if the following change can solve your issue?

C
diff --git a/llvm/lib/CodeGen/MachineSink.cpp b/llvm/lib/CodeGen/MachineSink.cpp
index 0abdf89..8ca3520 100644
--- a/llvm/lib/CodeGen/MachineSink.cpp
+++ b/llvm/lib/CodeGen/MachineSink.cpp
@@ -79,6 +79,12 @@ static cl::opt<unsigned> SplitEdgeProbabilityThreshold(
         "splitted critical edge"),
     cl::init(40), cl::Hidden);
 
+static cl::opt<unsigned> SinkLoadInstsPerBlockThreshold(
+    "machine-sink-load-instrs-threshold",
+    cl::desc("Do not try to find alias store for a load if there is a in-path "
+             "block whose instruction number is higher than this threshold."),
+    cl::init(2000), cl::Hidden);
+
 STATISTIC(NumSunk,      "Number of machine instructions sunk");
 STATISTIC(NumSplit,     "Number of critical edges split");
 STATISTIC(NumCoalesces, "Number of copies coalesced");
@@ -1036,6 +1042,12 @@ bool MachineSinking::hasStoreBetween(MachineBasicBlock *From,
     HandledBlocks.insert(BB);
     // To post dominates BB, it must be a path from block From.
     if (PDT->dominates(To, BB)) {
+      // If this BB is too big, stop searching to save compiling time.
+      if (BB->size() > SinkLoadInstsPerBlockThreshold) {
+        HasStoreCache[BlockPair] = true;
+        return true;
+      }
+
       for (MachineInstr &I : *BB) {
         // Treat as alias conservatively for a call or an ordered memory
         // operation.

Unfortunately, this patch doesn't work. I don't think the number of blocks is necessarily related to the number of instructions.
For one function, with your default threshold, The time spent on Machine code sinking is 11.2464s, no obvious difference from before. When set the threshold to 1, the time spent on Machine code sinking reduced 0.6937.
I think it would be better if you can limit the number of MIs.
Anyway, this patch provides us with an option. Thanks for your help.

In D86864#2472369, @LiuChen3 wrote:

I can surely do that. But I think the most reasonable solution would be fix the compiling time issue. Since compiling time tests I did before does not expose any regression, your test case must be a little special. Could you find out the special point, for example the function has too many blocks or some/many blocks in the function has too many instructions? Thanks.

I think the increase in compile time is because the function has too many instructions and blocks. The function has Thousands of lines of instructions. Can you add limitation for the number of instructions or the number of blocks so the check for 'store' can end early?

gentle ping

fix lint warnings

In D86864#2469576, @LiuChen3 wrote:

Yes, we have been aware that this patch may introduce compiling time degradations. And as you can see in previous comments, I already tested the compiling time on X86 arch. Sadly, the tested benchmarks don't expose any regressions.

Could you please help to send me your regression function/IR? So I can have a look about how to fix it? Thanks.

I am sorry that I can't provide the case to you directly. I am trying making a small reproduce but encountered some problems.
However, can you adding an option to control it?
Thanks.

In D86864#2469289, @LiuChen3 wrote:

Hi, @shchenz. Our several opecncl benchmarks have appeared great compile time regression.
For only one function, the time consume on Machine code sinking pass increased form 6.0711s to 366.5713.
According to your algorithm, this patch will obviously increase the compile time for some cases.

In D90807#2466923, @nemanjai wrote:

Thank you so much for fixing this and I'm sorry I didn't get around to going over it sooner.

perf test shows some degradations together with some improvements. Plan change for now to get more tuning.

Since @efriedma has concerns about adding more hook in machineLICM pass, and the RA works as expected(@qcolombet please correct me if you find there is any issue in RA process in above comments), I made a new solution for this issue:
instead of hoisting all remat instructions, we now do:

// For remat instructions which are inside current working loop, we should
// always hoist them.
// For remat instructions which intend to be hoisted to outer parent loop, we
// only hoist non-cheap ones as RA can not pull all remat instructions down to
// inner loop as it will first try to split them in outer loop.

@qcolombet @efriedma , sorry for the late response and the long main. I made a summary for this issue.

address @arsenm comments:
1: handle the case when hoisted and dup registers are both dead.

address @arsenm comments:
1: do not use const_cast
2: add comments for the test point in test case.

update the comments and gentle ping

I should have said this in my first comments. I like this refactor. I just want to make sure I understand this refactoring more clear^_^. Thanks for your detailed explanation. @nemanjai

update according to @jsji comments

The primary motivation is that the current implementation of selecting load/stores is dependent on the ordering of patterns in TableGen.

I think it is better to explicitly give some reasons why we need this big refactoring, in other words, what's the disadvantage/limitation of legacy implementation? Thank you for the big effort.

Thanks for clean-up. Some minor comments.

1: add side effect flag for instructions too.

gentle ping...

Thank you! @spatel

1: update according to parent patch https://reviews.llvm.org/D92068 changes

In D92068#2446159, @spatel wrote:

If this is NFC now, no objections from me - I just noted minor errors.
So we do not need LiveIntervals analysis at all now to reduce pressure, or that will be dealt with separately?

1: address @spatel comments

1: don't require LiveIntervals analysis pass to estimate register pressure.

Hi @spatel @lebedev.ri , I changed the patch not requiring LiveIntervals pass. Now this patch is NFC.

1: don't require LiveIntervals analysis
2: revert the test case change as now this should be NFC

gentle ping...

gentle ping...

Thanks for your comments @spatel @lebedev.ri

1: fix comments

@spatel Very much appreciate your comments. It is a good start for this patch to move on.

gentle ping

lint warning fix

1: fix lint warning messages
2: fix x86 CodeGen/X86/coalescer-dce.ll verify error after liveintervals introduced in machine combiner pass

thanks for your review, I will update the lint warning in the commit

This is more clear than the previous version. Some implementation comments.

gentle ping

LGTM. Thanks for the new fix.

After a discussion with the group I would like to correct what I said in the previous post.
There already is a plan to do this in ISel in a different patch. The reason we also want to do this optimization here is to try to catch situations where this pattern is not known in ISel and only appears after other optimizations later on. Ideally we do not want to have any situations where the XForm exists in the final binary and having this final check in the PreEmitPeephole should ensure that. Basically, we also want to do this check here to find anything that ISel may have missed.

In D89855#2399063, @Esme wrote:

In D89855#2398960, @shchenz wrote:

I saw some testcases are removed compared with your committed version. Is there any reason?

Every specific folding patterns with different masks have been tested in llvm/test/CodeGen/PowerPC/fold-rlwinm.mir
This patch didn't change the patterns, so I supposed it would be clearer to remove some redundant test cases.
Do you think they should be added back？

I saw some testcases are removed compared with your committed version. Is there any reason?

ping

Using dform with offset 0 can save one register r0/X0, this is benefit for register allocation? But adding it in PPCPreEmitPeephole pass which is after register allocation will make the benefit gone.
Maybe we need to do it before register allocation? For example at the place where the x-form with zero register is generated.