In D129065#3631052, @jrtc27 wrote:

Commit subject/body should say C++ in it

Update summary

In D128625#3621030, @MaskRay wrote:

The description seems unclear to me. Is a riscv64-unknown-linux-gnu GCC installation selected while the requested target triple is riscv64-unknown-elf?

Please rebase against precommited tests.

Please rebase against precommited tests.

In D122233#3513581, @RKSimon wrote:

FWIW we might be able to perform something similar inside VectorCombine

In D122233#3399664, @lebedev.ri wrote:

Do we expect that unused inputs of the shuffle has been already replaced with undef?

This is already closed, reopened accidentally.

Abandoning this. PR52275 was solved by D119679 and following patches.

In D109817#3344942, @lebedev.ri wrote:

In D109817#3344766, @anton-afanasyev wrote:

Fix sanitizer breakage: erase phi-nodes from InstInfoMap before erasing themselves

(If you believe that fixes the issue then feel free to recommit, no further feedback here expected)

Fix sanitizer breakage: erase phi-nodes from InstInfoMap before erasing themselves

I'm to backport this: https://github.com/llvm/llvm-project/issues/54013

@lebedev.ri Ping. Is it ok now?

This is a partial revert of rG954ea0f044e0. It looks like there is still an issue with cost computation here, but I'm to fix it in a separate patch and to backport both patches to 14.0.0. Therefore looks good to me. Add @ABataev.

LGTM

Add test

In D109817#3320059, @lebedev.ri wrote:

In D109817#3319994, @anton-afanasyev wrote:

Are there tests with switches, where the PHI node has multiple incoming edges from the same predecessor?

What do you mean by "the same predecessor"? There is test with ordinary loop.

https://godbolt.org/z/MdsrzGnvq

Add comment

Are there tests with switches, where the PHI node has multiple incoming edges from the same predecessor?

What do you mean by "the same predecessor"? There is test with ordinary loop.

Address comments

In D119623#3318056, @ABataev wrote:

Would be good to test it using test suite at least, just in case we missed something.

Interpret insertelements with undef/uncorrect/non-constant index as initial vector

Well, that would definitely solve the problem. There is also another way: on the contrary, remove hasOneUse() check from vectorizeInsertElementInst. This patch allows it.

Yes, sure, but it will require some extra stuff and some time to polish and fix all corner cases. If we’d like to fix it in 14.0, better to start with a simple fix and then try to implement more complex feature.

Ok, got it. Could you also add a check to the buildvector detection function for such kind of insertelement (+ possibly insertvalue) instructions? They should not be treated as a part of buildvector, need to treat them as an initial/terminating instruction and exclude from possibly vectorizable buildvector instructions.

In D119623#3316685, @RKSimon wrote:

Something to take it account with this and D119536 - we're going to need to cherrypick these fixes for PR52275 as they affect 14.x as well - will they be able to apply cleanly to the 14.x branch?

No, they doesn't apply cleanly right now, but I'm to backport them through new workflow (https://discourse.llvm.org/t/rfc-new-automated-release-workflow-part-2/59981)

In D119623#3316827, @ABataev wrote:

What kind of bugs do you see with these test cases and current version of the function? I would try to keep it, better to generate UndefMaskElem if possible, rather than cut of the vectorization early.

Fix rebase

In D119536#3314640, @ABataev wrote:

Hmm, looks like the problem appears only if we have insertelements to be vectorized not from vectorizeInsertElementInst, where we have a proper check. Maybe just add a check for insertelements in other functions and just expect them from vectorizeInsertElementInst?

Address some comments, rebase against D119623

Make a separate NFC patch for default parameter of getInsertIndex

I've investigated small regressions, caused by this patch. The sizes are actually increased at the backend level, while translating llvm to machine code, for instanse:

> ll symtab.c.*
13769 symtab.c.base.ll
13681 symtab.c.exp.ll
8280 symtab.c.base.s
8394 symtab.c.exp.s

For the tests from testsuite this mostly leads to just +16 bytes code size increase (see table at D109817#3184752), so I believe it's acceptable fluctuation.

Rebase

In D108201#3249134, @fhahn wrote:

Not sure if it has been raised elsewhere already, but it looks like this is causing a regression: https://github.com/llvm/llvm-project/issues/51922

In D109817#3184760, @lebedev.ri wrote:

I would recommend to proceed with this.

In D109817#3060903, @RKSimon wrote:

In D109817#3043432, @anton-afanasyev wrote:

Ping.
Though this patch makes AIC more complicated (using general graph instead of expression DAG), it spreads AIC outside one BB. It impacts ~30 tests of test suite.

Are there any notable diffs (codegen/perf) in those tests?

Update tests

In D113179#3147047, @spatel wrote:

In D113179#3146480, @RKSimon wrote:

any more comments? @lebedev.ri @spatel

I'll make my earlier acceptance official here in Phab.
I can't find any earlier reviews/comments to justify the ordering, there are no regression tests to support it, and we have evidence that this (new/proposed) ordering helps at least one case (and we still have a bug tracking the root cause).

Btw, the only impact seen by this patch is almost zero, the test suite shows three size-reduced tests and no regressions:

$ ~/llvm/test-suite/utils/compare.py -a --filter-short -m size..text result_base.json vs result_exp.json
 ...
 test-suite...abench/jpeg/jpeg-6a/cjpeg.test   167826.00  167810.00  -0.0%
 test-suite...nsumer-jpeg/consumer-jpeg.test   167186.00  167170.00  -0.0%
 test-suite...lications/sqlite3/sqlite3.test   496674.00  496546.00  -0.0%

Bu general recollection is that the order is intentional
to avoid pattern disturbance by instcombine before AIC has a chance.

Hmm, it would be great to see an example. I've seen the reverse cases for now: disturbance by AIC before IC.

There is no cost difference, just the tests are crashing.

Ok, I see, so add such test to this commit?

Add test showing different costs?

Thanks for digging that up. I don't see any reason that it was placed that way either, so LGTM. Please wait to see if @lebedev.ri agrees though.

Ping @lebedev.ri .