It would be ok for me if you reverted it while investigating regressions. I would hope then that you would try the "partial revert" meaning removing just the change in NotProfitableForVectorization().

Thanks for trying the idea of a single map!

Patch updated per review.

In D123394#4191917, @vpykhtin wrote:

Hi Valery, thanks for the report! It would be nice if you posted your test case on github issues and assigned me (and/or yourself :)... I would think this could be fixed.

Sorry for delay, done https://github.com/llvm/llvm-project/issues/61397. Maybe if you have the version with kill flag maps we can check if it works better on the testcase.

Patch updated per review - see inline comments.

Updated patch per review.

New tests:

I have experimented with this here: https://reviews.llvm.org/D146425

In D144128#4199494, @fhahn wrote:

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?

To add to what @zjaffal said, I am curious why the current cost-modeling wouldn't be sufficient to prevent cases where vectorizing GEPs would be not profitable?

The case @zjaffal shared requires a more costly vector GEP, but the cost is offset by the benefits from vectorizing the rest of the tree.

Ping - does it look ok to emit a warning like this from CodeGen?

In D145811#4192439, @nikic wrote:

LGTM

ok, I think I got it now: we can drop the NW flags because the backend will then later properly handle OF as well during comparison elimination (and the program prints the right result).

In D123394#4186531, @vpykhtin wrote:

Makes sense to me - I tried also earlier to store the kills in a map but I found (at least on SystemZ) that this backwards search was actually quicker than that, so I don't think it should be an issue in the normal case...

Hello Jonas, just FYI I got case:

Total Execution Time: 670.4078 seconds (670.7149 wall clock)

   ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- Name ---
  289.2998 ( 43.7%)   0.1439 (  1.8%)  289.4437 ( 43.2%)  289.5775 ( 43.2%)  Structurize control flow
  124.9852 ( 18.9%)   0.0000 (  0.0%)  124.9852 ( 18.6%)  125.0289 ( 18.6%)  Machine Late Instructions Cleanup Pass
  45.2408 (  6.8%)   0.0000 (  0.0%)  45.2408 (  6.7%)  45.2740 (  6.8%)  SI Form memory clauses
  38.0358 (  5.7%)   0.0240 (  0.3%)  38.0598 (  5.7%)  38.0767 (  5.7%)  Simple Register Coalescing

Most of the time is spent in clearKillsForDef.

ping!

Usually for this sort of warning, we provide diagnostic notes: how should the user fix their code if it's wrong? How should the user suppress the warning if it's wrong?

I added a hint in the warning message to the user that __atomic is the way to go. The warning is enabled with -Wsync-alignment, which is also part of the warning message.

Better to calculate scalarization overhead for the loadinst elements and subtract from the gather cost.

Or better to include this check to the getGatherCost function

ping - everybody happy with this now?

Patch rebased with updated X86 tests. minimum-sizes.ll did no longer update automatically with "CHECK" prefix, so I added different prefixes to make the update succeed.

Patch updated to only give a warning.

In D144128#4131910, @ABataev wrote:

Could you check the throughput of (some of) the tests with and without your changes in godbolt?

Can you try to move this check to buildTree_rec function, NotProfitableForVectorization lambda and make it return true if S.getOpcode() == Instruction::GetElementPtr && !TTI->prefersVectorizedAddressing()?

Thanks for review!

Pushed on release/16.x branch as ca7d6a6.

Added i128 alignment change.

Thanks for explanation!

In this case I think it would be preferable to omit the attribute - it is completely safe to do so, even if it doesn't match GCC's behavior; in fact, we'd rather change GCC to match as well.

Yes, it's probably best to match GCC behavior here.

The old behavior is not explicitly specified by any ABI doc, so we can really only go after what GCC actually used to do. And in fact GCC used to (and still does!) simply place such arguments on the stack and passes a hidden pointer to the stack slot - without any dynamic stack realignment, so it in fact only guarantees 8 byte alignment. So I think it is *safe* to assume that using 8 bytes always is OK - and in that sense it is safe to omit the GNU attribute.

I see this with latest clang:

Review addressed - hopefully this time with a better result.

Ping!

Patch updated to also handle

LegalizeTypes expands the i128 result of e.g. an AtomicLoadAdd node, which is done by DAGTypeLegalizer::ExpandAtomic(), which results in a library call. There are no tests for the atomic operations with i128, but it should work due to the TypeLegalizer. i128 is not a legal type so I guess therefore they are all expanded this way.

Now a different question is the alignment of a simple v8i32 passed via implicit reference. Here, we should be passing a pointer to v8i32, so arguably GCC does indeed have a bug if that pointer doesn't have the natural alignment for its type. This probably doesn't matter however, as that implicit pointer is hidden and cannot be inspected by user code, so it would be impossible to actually detect any misalignment ...

I think this isn't quite correct - if Bytes >= 16, the alignment of the vector type is different, which may always be ABI-relevant, even when passed as argument. (E.g. when passing a (pointer to a) struct containing a vector member, the layout of the struct becomes ABI relevant, and that layout depends on member alignment.)

ping - any opinion on how to best determine the right extension actions for each target? Currently TLI holds variables (ShouldExt.../ShouldSignExt...), but TLI is not available in OMPIRBuilder. As discussed earlier, one option would of course be to provide TLI in OMPIRBuilder, but the alternative that I have found is to have a static function instead that just takes the Triple for each query. I think that would simplify things a bit (if always used) and I imagine it shouldn't be any compile time concern. What do you think?

We haven't enabled subreg liveness. What we do have though is a downstream hack in VirtRegMap.cpp that throws away implicit uses of superregisters since we haven't understood what they are good for and they just limit scheduling later.

Ah - yeah have some vague notion of that :-)

• Incorporated the recent change of sign extending i32 returns for RISCV.

Thanks for review. Extension of __tgt_target_data_begin_mapper_issue arg changed to SExt. I will wait for you on the other points...

Spent some time now on adding all the extension attributes in these passes per previous discussions.

Sorry - I disabled this already for NVPTX and WebAssembly for the same reason but must have missed that SPIRV does not do regalloc when no SPIRV tests at all changed.

Hi @uabelho , I'm glad you may find some use for this pass as well :)

Getting back to this now finally, sorry for the delay.

Thanks for review - patch updated. Tests vec-abi-gnuattr-14.c and vec-abi-gnuattr-20.cpp strengthened accordingly.

Latest patch as committed (did not update correctly automatically).

Reverted again with 122efef.

Recommitted as 17db0de, with RISCV disabling it for now.

OK - I will recommit this then as it does seem like a problem in the RISCV backend. I'll wait a day or so to let other targets check that they don't have this potential problem...

LGTM - I think now we are on the safe side regarding the chains.

Patch reverted because of build failure, relating to capturing structured bindings, which builds fine with gcc but not yet with clang.