Thanks for the update. Just one minor comment inlined.

Thanks Vedant for handling this. Overall I agree with this change, but I believe it's good to break this patch down into three parts and get each of them reviewed separately :

1. Mark @llvm.trap cold.
2. Do not treat noreturn calls as cold unless they are actually marked cold.
3. Do not treat inline asm as an outlining barrier.

Or

1. Mark @llvm.trap cold.
2. Do not treat noreturn calls as cold unless they are actually marked cold, and Do not treat inline asm as an outlining barrier.

Thanks for revisiting this.
LGTM.

Thanks Florian for the update.

I guess it would be good to give some stress test on this pass to see if there is any hidden bug by relaxing conditions in unlikelyExecuted(). As we treat more blocks as cold without being limited on unlikelyExecuted(), we maybe able to expose hidden issues with it.

IMHO the right fix is to not treat noreturn calls as cold (also as a follow-up).

Kindly ping one more time.

Kindly ping.

With the last modification I made based on Eli's comment, I didn't see any significant changes in size / performance in my spec2000 test on AArch64. Observed minor performance improvement in some internal benchmarks.

Tried to handle Eli's comment. Please take a look and let me know any comment.

Do you have perf/size numbers?

I didn't see any degradation in size/perf in my spec2000. Just observed minor performance improvement in some internal benchmarks.

LGTM

Compile it with clang -target hexagon -O3. The code you added eventually punts (FormInputLSRUseAndFormula returns false), and LSR proceeds to do its thing. I did some analysis, and the problem is with %add.ptr = getelementptr inbounds i32, i32* %2, i32 %Col.0. This is not an "address use", since it goes into another GEP. It exists in the original source, but LSR never looks at it. Your code does and that makes it exit early. Maybe you should restrict the uses you look at to the same ones that LSR starts with?

Sorry for the extremely long delay on this change. Now I just updated the patch. Please take a look and let me know any comment.

LGTM too. DomBBUsedRegs and OptBBUsedRegs are not really used, but keeping both DefedRegs and UsedRegs seems clearer than creating extra functions in LiveRegUnits.

Addressed Michael's comment.

Thanks Daniel for the review.
I agree that ideally LICM should preserve the CFG, and this change could potentially impact the compile time. However, I think this is the safest fix at this moment without a mess of updating extra analysis passes. In current pipelines (-O3, -O2, -O1, -Oz, and -Os), I looked at -debug-pass=Executions, and didn't see any extra execution of analysis passes after this change.

Refactors this change based on current tip. Made the check for load from FI more conservative.

Thanks Sebastian for your review!

Kindly ping.
Matthias, please let me know if this change is still okay with you.

Kindly ping?

Now reassociations lookup is bounded only by Depth. If there are reasonable amount of reassociations on each level ~16, the whole number would not exceed ~16^3 which is ok.

Thanks Quentin for the review.

Just a minor change in accumulateUsedDefed(). Please let me know any comment.

Hi Matthias,
Please let me know if my change in LiveRegUnits is okay with you.
Thanks,
Jun

Addressed Matthias' comment by moving TargetInstrInfo::trackRegDefsUses into LiveRegUnits::accumulateUsedDefed.

Have you seen LiveRegUnits::accumulate() glancing over the code here (and earlier) it seems to me that TII::tracksRegDefsUses() was just invented to do the same thing: figuring out which registers are free/usable over a range of instructions...

Thanks for handling this. Just curious if you see any issue with the non-null attribute in a constant ?

Addressed Francis' comment.

Even without LiveRegUnits, I think you could replace ModifiedRegs and UsedRegs to be ModifiedRegUnits and UsedRegUnits, and use MCRegUnitIterators to check if registers alias.
I'm ok to commit this and follow-up in separate patches for the rest. Thanks!

My initial motivation case for this was when spilling the incoming argument register. This is somewhat related with the initial motivation of PostRASink pass because we do not sink the COPY for argument register before allocating. When RA try to spill the incoming argument register, it probably don't want to change the placement of the spill during RA, since sinking it down will extend the live range of the spilled value. So I do this after RA just like we did it for Copy in PostRASink pass.

Just minor update in comments.

What do you think about using register units throughout the pass? I found them so much clearer to use when dealing with register aliases.

With/without this change, I collected llvm stats for spec2000/2006/2017 on AArch64. I observed +31.87% more loads from stores promoted in AArch64LoadStoreOptimizer pass, and minor improvement in shrink-wrapping in case spills are sunk from the entry.

Ping ?

Added NoVRegs for ShrinkWrap as well.
Updated summary and title.

Thanks for handling this. Just minor comments inlined.

The shrink-wrap pass would already assert here. Could you add the property to ShrinkWrap as well?

Sounds good to me.

Thanks for doing this. Now, you intended it to be a NFC, right? If then, it will be good to keep the original test cases as it is. Adding more tests is definitely good.

Rebased and minor changes in comments. Please let me know if the approach I'm using here make sense.

Wow, thank you so much for fixing this.

This pass destroys DominatorInfo and we have to recompute it right after the pass from scratch. Is it possible to preserve it? Also, have you measured compile time impact of the patch?

Assuming this pass will be disabled on Hexagon, XFAILed swp-phi-ref.ll just like noreturn-noepilog.ll.

Found a new failure in a recently added hexagon test.