In D144128#4202886, @jonpa wrote:

Sorry about your regression. I looked for a runline in the "godbolt" link, but could not find one.

LGTM with the TODO in the test removed, thanks!

ping :)

Ping :)

Just a heads up with are seeing a 10% regression caused by this change in a very SLP sensitive workload (the original source for the slp-fma-loss.ll tests). I still have to double check where the slowdown is coming from exactly.

LGTM, thanks for fixing this!

@cmatthews @azhar do you know if there are any tests for this part of the code?

Looks like multiple existing tests are triggering the assert and are failing.

Fix LGTM with comments by other people addressed.

LGTM, thanks for splitting this off!

In D144128#4199448, @zjaffal wrote:

Hi this patch is causing some regressions.
If you look at the example I attached the code sequence is no longer being vectorised when it is beneficial to do so.
https://godbolt.org/z/MTex1z73K

Can you take a look please?

The changes to make the tests more robust should probably be submitted separately so the diff only shows the functional test changes. Also, would be good to pre-commit new tests like vector_add_trip1024

Please make sure to update the commit message; it looks like the latest version doesn't address the poison issue IIUC

LGTM, thanks!

Address comments, remove ifdef code for ppc and adjust shadow-memory used on ARM64 macOS.

LGTM, thanks!

LGTM, thanks!

In D145677#4185360, @vitalybuka wrote:

In D145677#4184405, @fhahn wrote:

Thanks for the fix @vitalybuka ! I am surprised this wasn't covered by the added tests. Any idea why that is?

I guess you need shift by [32; 63] to hit this case

Update to use LLVMContext::MD_nosanitize , add nosanitize.ll test.

Bail out if global initializer cannot be evaluated (test added in a011823bff4d5399).

Thanks for the fix @vitalybuka ! I am surprised this wasn't covered by the added tests. Any idea why that is?

LGTM, thanks!

LGTM. thanks! But please include the full test (from D145676) in the commit with the fix, as the tests will otherwise fail on the UBSan bots.

LGTM, thanks!

Only call ConstantFoldLoadFromConst when new entry has been inserted.

In D145489#4175498, @dblaikie wrote:

Sounds good to me - wouldn't even mind if you wanted to implement a generic abstraction, less<N> and implement less_first/second as aliases of that type.

Address latest comments & rebase before landing, thanks!

Rebase before landing, thanks for the review!

It looks like the test failures were from earlier runs, possibly with a missing dependency. I re-run them and they came back clear (on Windows, ClangdTests.exe/IncludeCleaner/GenerateMissingHeaderDiags failed, but that's very likely unrelated)

LGTM, thanks!

LGTM, thanks! The range is guaranteed to not include undef as per an earlier check.

LGTM, thanks!

LGTM with the suggestions addressed before landing.

In D144476#4171586, @nikic wrote:

LGTM, though I think it might be cleaner if we collected the initializer part during the construction of GlobalPart, rather than calculating it twice, here and during the SRA split.

LGTM, thanks! The commit title could be a bit more descriptive by explicitly mentioning what the code change is, e.g. something like: [LV] Use SetVector to collect invariants in calculateRegisterUsage.

In D144101#4174512, @rengolin wrote:

In D144101#4174508, @fhahn wrote:

It would probably be even better to make the threshold an argument (perhaps pass the threshold optionally like --filter-short=N), with the default being 1.0?

I like this idea. Where would the argument come from in the buildbots? CMake?

It would probably be even better to make the threshold an argument (perhaps pass the threshold optionally like --filter-short=N), with the default being 1.0?

What are the current thoughts on the next steps to move forward with this. As for the 2 regressions, I think the one in llvm/test/Analysis/ScalarEvolution/incorrect-exit-count.ll won't be easily resolve-able. The other one can be solved by a follow-up (D144753), but it needs a bit more work because currently it triggers an infinite loop in a test case.

LGTM, thanks for updating this!

rebased and removed MIPS and PPC from compiler-rt/lib/tysan/tysan_platform.h as it has not been tested.

Rebased

Address outstanding comments, remove some leftover commented-out code.

Use existing offset directly.

Rebase on top of 0ecef88.

Update to use ConstantFoldLoadFromConst to check the initializer value in the specifi field.

Not sure if replace_undef_X is an ideal name. Might be better to use something short like cX or condX

In D144125#4151066, @michaelmaitland wrote:

In D144125#4150975, @fhahn wrote:

After changing VPWidenIntOrFpInductionRecipe, are the other changes still needed?

I think this change reduces class bloat (since not all recipes are phi like, header phi like, or generate backedge). It also makes it so we only use RecipeTy in classof and uses isa to determine whether something isPhi.

I have some changes I plan to upstream in the near future that I think will give these util functions an even stronger justification for existing. If you don't think that the justifications i give are strong enough now, I am happy to remove them from this patch and include them in the future patch I mentioned.