Thanks for the review! Will commit this version.

In D145614#4224190, @dmgreen wrote:

In D145614#4214202, @Carrot wrote:

X86 backend also set store of v4i8 as custom. We have similar capability for v4i8.

I can also add a real custom way of lowering store of v4i8 as following. It's more simple and natural than storing of v4i16.

t2: i32 = bitcast t1:v4i8
t3: ch = store<store (s32) into %somewhere> t10, t2, address

But v4i8 is not a legal type, so we can't see a store v4i8 dag node, so it looks not necessary.

I believe that X86 will treat vector lanes differently to Arm/AArch64. For smaller types the vector will be widened by adding more elements (v4i8 [a,b,c,d] will become v8i8 [a,b,c,d,u,u,u,u]) as opposed to being promoted under aarch64 to larger sizes (v4i8 is promoted to v4i16, with the top half of each lane unused). They can both have their advantages and disadvantages. With SVE having t/b instructions the promotion can make more sense, and it is good to keep SVE and NEON inline. The same is true under MVE which only has 128bit vectors so more types are promoted, but this plays nicely into the how the t/b instructions operate.

Override getStoreMinimumVF to support vectorization of store v4i8 for AArch64.

X86 backend also set store of v4i8 as custom. We have similar capability for v4i8.

@nikic, the original version of this patch is 0.07% slower because more triggered optimizations caused more iterations on huge functions.
http://llvm-compile-time-tracker.com/compare.php?from=d38d6065584ee5dd837e9a629f90c731d8a7dffc&to=94fc2022ff32b63d6c744d3eeff4f304e1a81618&stat=instructions:u

ping

I tried isTruncStoreLegalOrCustom and it works, generates the same code as my patch. The final code is

Current compile time change
http://llvm-compile-time-tracker.com/compare.php?from=c8e5354f0063b090b6fa7ad4cfc2b9df78038454&to=d21dbfa65423bbd9251ab5948f6cc1baa2db61c8&stat=instructions:u

To improve compile time, now I record the basic blocks contain dead load instructions, at the end of the same iteration, process these blocks again to delete load instructions. So we can avoid the extra iteration on the whole function.

In private email communication, @hans told me he couldn't find any problem in his optimized code, and his fail disappeared after updating his source code. So there is no known problem with this version.

Check for implicit control flow instruction in function findLoadToHoistIntoPred, so we can avoid moving load across unexpected control flow.

It's reverted now. Thanks for the test case. Will improve it in another version.

@reames, I'm happy to revert it once I get your test case, otherwise I have nothing to investigate.

@hans, do you have a reduced test case to demonstrate the problem?

In D142787#4090389, @mkazantsev wrote:

Can we follow-up updating the place I've pointed out above?

@uabelho, thank you for the reporting! I think you are right. I'm preparing the patch.

In D141712#4082217, @nikic wrote:

It looks like the new version of the patch ended up being more expensive in terms of compile-time: Original, New

I wonder whether this is because not removing the load essentially always forces an extra GVN iteration?

In D141712#4062475, @kachkov98 wrote:

In general, I'm concerned that this patch tries to combine 2 different transformations: load PRE and hoisting. It can omit critical edge splitting in some cases, but after hoisting we can also cleanup CFG.

@SixWeining @dyung , both reproduction work for me, thanks a lot!

@dyung @chapuni thank you for the report, I have reverted it.
How did you investigate it ? I try to reproduce it according to https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild, but always got following error

...
+ BUILDBOT_MONO_REPO_PATH=
+ BUILDBOT_REVISION=llvmorg-
+ buildbot_update
+ echo @@@BUILD_STEP update llvmorg-@@@
@@@BUILD_STEP update llvmorg-@@@
+ [[ -d '' ]]
+ local DEPTH=100
+ [[ -d llvm-project ]]
+ cd llvm-project
+ git fetch origin
+ git clean -fd
+ local REV=llvmorg-
+ git checkout -f llvmorg-
error: pathspec 'llvmorg-' did not match any file(s) known to git
+ git status
On branch main

Thanks for the review! Will commit this version.

Rebase the code.

The compile time regression on ClamAV is in file libclamav_htmlnorm.c. The increased compile time is completely in GVN pass, from 4.0s to 5.3s. There is a huge function cli_html_normalise in this file, it's more than 6600 lines in generated assembly file. The statistic result of NumPRELoad increased from 1 to 10 because of this patch. In function GVNPass::runImpl we have

unsigned Iteration = 0;
while (ShouldContinue) {
  LLVM_DEBUG(dbgs() << "GVN iteration: " << Iteration << "\n");
  (void) Iteration;
  ShouldContinue = iterateOnFunction(F);
  Changed |= ShouldContinue;
  ++Iteration;
}

It means we continuously do GVN on a function until there is no more such optimization applicable. This patch enables more optimizations, potentially it may also cause more iterations on a function. In this case, the loop is executed 4 times without this patch, but 5 times with this patch. These numbers closely correlate to the increased compile time.

Add a test case to show that llvm can generate wrong instructions without this patch.

Compile time improvement.

In D139582#3981256, @mkazantsev wrote:

Seems like this patch keeps finding values in vectors, and it might be a reason of CT degradation. Maybe refactor them to be maps instead?

Any other comments?

ping

ping

Rebase this patch after committing the MachineCSE part.

ping

MachineCSE was changed recently, so rebase this patch. Now it has more impact.

@foad, thanks a lot for reviewing AMDGPU changes!

In D130919#3891206, @foad wrote:

Why are there so many changes in AMDGPU codegen, even in tests that do not contain function calls?

Any other comments?

In D136396#3886403, @JDevlieghere wrote:

Hey, this broke a Dexter test: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/47878/

@jmorse and/or @Orlando can you help make sure this isn't a Dexter issue?

Rebase test cases.

Thanks for the review. Will commit this version.

Rebase.

ping