Update to use constant policy.

Since there is a bug and a repro case, I'd like to ask if this change can be rolled back until a proper fix is in order.

I've gotten a repro case. Created https://bugs.llvm.org/show_bug.cgi?id=50709 with details. Please let me know if I should replicate details here instead.

In D104028#2818310, @taolq wrote:

In D104028#2817188, @mtrofin wrote:

With the introduction of the flag, there shouldn't be any more failing tests, right?

Yes, all tests pass.

In D104028#2817188, @mtrofin wrote:

With the introduction of the flag, there shouldn't be any more failing tests, right?

clang-format fix.

Updated list of failures; it's now going up...

I don't think this is general enough requirement to add a specialized method for it. Did you consider adding a consumes_front/back overload that takes a lambda?

In D104060#2818277, @davidxl wrote:

Good summary.

For instance, the important Kirichoff's circult law (aka spanning tree)
optimization is not implemented. (I added the optimization to clang
-fprofile-generate).

You probably meant -fprofile-instr-generate :)

In D104007#2817612, @efriedma wrote:

However latter optimization passes could very well optimize out that copy because the copy would appear to be useless without considering what coro_suspend does, which is why we are changing BasicAA to prevent such elimination.

If an argument is just a pointer without any argument attributes, BasicAA will assume coro_suspend frees the underlying allocations. Not because it has any particular knowledge of how coro_suspend works, but because that's the default assumption for any function call with unknown semantics.

BasicAA treats arguments marked byval differently because it understands the semantics of byval: it uses escape analysis to prove an arbitrary function call can't modify the contents.

---

I guess in some sense, it's possible to specify a world where coro_suspend frees byval allocations. But such a world doesn't really make sense. And we'd need to audit every pass that knows anything about byval semantics, so I really don't want to go down that path.

C++ non-virtual thunks and global initialization functions (_GLOBAL__sub_I_*) have only linkage names.

Good summary.

friendly ping

only check byval attribute

I have played with the patch.

I did try to write an offsetof-based version using inheritance instead of the "common initial sequence" rule, but I am not sure how to do the necessary checks statically. The best I can see to do would be to have some dynamic ASSERTs to confirm that casting the alternative type to the base type does not change the underlying representation (i.e. if both pointers are cast to uintptr_t they are equal).

Made some changes suggested by Andrea.

@stella.stamenova do you still think you'd be able to test this before it lands, to see if the current version of the patch works on the MSVC build bots?

Ping

Thanks! I'll wait for another CI notification to post before landing.

• Add requires for clang to avoid unknown gcc flag errors

Is

maybe we should make a quick post on llvm-dev about this to see what other people think, since this will be very user-visible

Still broken...

In D104099#2815531, @wenlei wrote:

In D104099#2814167, @davidxl wrote:

Adding Wei to help measure performance impact on our internal workloads. Also add Wenlei to help measure impact with FB's workloads.

Measured perf using FB internal workload w/ and w/o this pass, result is neutral.

Add a test case.

In D104149#2817752, @holland11 wrote:

I just spoke with Quentin, and we both agree with you. If it's alright with @foad, we'd like to push the current patch with the AMDGPU example (after I finish making the modifications Andrea suggested in his first comment). Afterwards, I will submit a bug report regarding the relevant flags not being transferred from pseudo -> real instructions. This will allow me to clean up the cnt detection logic and make it a lot more similar to the logic found in the si-insert-waitcnts pass (which will make the s_waitcnt modelling more accurate).

I'm surprised by this. I don't see anything in the tablegen files to say that DS instructions write to M0.

This is interesting. The MCInst for DS_READ_U8_gfx10 definitely has a Def for M0.

		Opcode Name= DS_READ_U8_gfx10
		SchedClassID=4
		Resource Mask=0x00000000000004, Reserved=0, #Units=1, cy=1
		Resource Mask=0x00000000000008, Reserved=0, #Units=1, cy=1
		Buffer Mask=0x00000000000004
		Buffer Mask=0x00000000000008
		 Used Units=0x0000000000000c
		Used Groups=0x00000000000000
		[Def]    OpIdx=0, Latency=20, WriteResourceID=0
		[Def][V] OpIdx=4, Latency=20, WriteResourceID=0
		[Use]    OpIdx=1, UseIndex=0
		[Use][V] OpIdx=4, UseIndex=3
		MaxLatency=20

The M0 def corresponds to [Def][V] OpIdx=4, Latency=20, WriteResourceID=0 where the [V] signifies that it's a 'variadic' operand (I'm not entirely sure what this means).

I can submit a separate bug report for this if you'd like. It's not really related to the CustomBehaviour class or the s_waitcnt instruction.

In D97982#2818213, @Amir wrote:

In D97982#2811315, @efriedma wrote:

Which disables NeverAlign padding (0 bytes)

When do we actually do this computation? At first glance, MCAssembler::layoutSectionOnce never actually goes back to recompute the size of the NeverAlign padding.

So it's the case that layoutSectionOnce/layoutOnce actually recomputes the size of the NeverAlign padding indirectly:

Inside layoutSectionOnce, when it relaxes the fragment (relaxFragment), if the fragment is FT_Relaxable, it calls relaxInstruction, which in turn checks fragmentNeedsRelaxation for the passed fragment F, which then goes over F->getFixups, calling evaluateFixup, and so on through Layout.getSymbolOffset(Sym) and Layout.getFragmentOffset, inside the ensureValid check, calls layoutFragment, which then computes fragment size (computeFragmentSize):

Debugging into this a little further, it's unclear to me how EVT::changeVectorElementType() can be expected to work reliably: it assumes that if EltVT.isSimple() == true, then MVT::changeVectorElementType(EltVT) will be legal, but this is clearly not the case, since there aren't equivalent widths of MVTs for all element types. (In this case, v6i32 -> v6i16 isn't possible since v6i16 doesn't exist).

In D104156#2818072, @lebedev.ri wrote:

Many remaining cases are rotates, with the pattern like:

Combining: t0: ch = EntryToken
Optimized vector-legalized selection DAG: %bb.0 'splatvar_funnnel_v8i32:'
SelectionDAG has 26 nodes:
  t0: ch = EntryToken
  t2: v8i32,ch = CopyFromReg t0, Register:v8i32 %0
          t25: v2i64 = zero_extend_vector_inreg t45
        t26: v4i32 = bitcast t25
      t27: v8i32 = X86ISD::VSHL t2, t26
              t40: v4i32 = BUILD_VECTOR Constant:i32<32>, Constant:i32<32>, Constant:i32<32>, Constant:i32<32>
            t38: v4i32 = sub t40, t45
          t30: v2i64 = zero_extend_vector_inreg t38
        t31: v4i32 = bitcast t30
      t32: v8i32 = X86ISD::VSRL t2, t31
    t21: v8i32 = or t27, t32
  t10: ch,glue = CopyToReg t0, Register:v8i32 $ymm0, t21
        t4: v8i32,ch = CopyFromReg t0, Register:v8i32 %1
      t6: v8i32 = vector_shuffle<0,0,0,0,0,0,0,0> t4, undef:v8i32
    t43: v4i32 = extract_subvector t6, Constant:i64<0>
    t47: v4i32 = BUILD_VECTOR Constant:i32<31>, Constant:i32<31>, Constant:i32<31>, Constant:i32<31>
  t45: v4i32 = and t43, t47
  t11: ch = X86ISD::RET_FLAG t10, TargetConstant:i32<0>, Register:v8i32 $ymm0, t10:1

Let's suppose we start at t27: v8i32 = X86ISD::VSHL t2, t26, and demand the 0'th element of shift amount t26.
I think the problem is that t45 has another use - t38,
for another shift. Likewise, if we start from the other shift.
I'm not sure if this can be solved within the existing demandedelts infra?

Fixed backwards compatibility problem introduced by previous version of the patch.
That is restored the size of task dependence flag to 8 bits in clang, and instead changed the size of the flag in the library from 32 to 8 bits.
Fixed two tests so that they set all 8 bits of the flag.

In D97982#2811315, @efriedma wrote:

Which disables NeverAlign padding (0 bytes)

When do we actually do this computation? At first glance, MCAssembler::layoutSectionOnce never actually goes back to recompute the size of the NeverAlign padding.

Reorder variable definitions to reduce line count

Disable macos test

MIP does not support windows

That is interesting. The cmake does require opt (and llvm-link), and the only reason it does not specify a dependency on those is I do not know how to spell that in cmake.

In D99696#2817126, @rjmccall wrote:

When __block variables get moved to the heap, they're supposed to be moved if possible, not copied. It looks like PerformMoveOrCopyInitialization requires NRVO info to be passed in to ever do a move. Maybe it's being passed in wrong when building __block copy expressions in some situation.

Found the culprit, that functionality was implemented in checkEscapingByref, and it was a bit of a shocker that such a thing was just hiding in a function with such an unassuming name :)
Also surprising to find here another user of the two-step overload resolution, that also escaped our radar.
There is a C++ proposal (P2266) to replace that mechanism with something much simpler.

Fix build failures when using -DDBUILD_SHARED_LIBS=On. Thanks @MaskRay

there needs to be tests, and the description should probably state what's going on

Thanks a lot! Indeed, we don't have access to FileCheck in the libc++ tests. I had once looked into doing that but I ran into issues and we didn't have a clear use case so I decided not to spend more time on it. Looks like this will do for the time being.

I also plan to add patches for:

• fixing the handling of load/store instructions in various places where a single (for load) or double operands (for store) are assumed.
• introduction of an intrinsic that combines the pointer provenance with the pointer value (Corresponding to the llvm.noalias.arg.guard in D68487)

In D103881#2817686, @srj wrote:

I'll be adding more details and adding a repro case as soon as I have a small one; posting this now to see if anyone else has noticed a regression/injection.

Good catch on the end.cpp rename - we were not running that test.

With Diff 351900 of this patch applied, a RISCV build of the mainline Linux kernel with LLVM=1 LLVM_IAS=1 builds and boots. This patch resolves https://github.com/ClangBuiltLinux/linux/issues/1023 for me.

LGTM

LGTM

Many remaining cases are rotates, with the pattern like:

Combining: t0: ch = EntryToken
Optimized vector-legalized selection DAG: %bb.0 'splatvar_funnnel_v8i32:'
SelectionDAG has 26 nodes:
  t0: ch = EntryToken
  t2: v8i32,ch = CopyFromReg t0, Register:v8i32 %0
          t25: v2i64 = zero_extend_vector_inreg t45
        t26: v4i32 = bitcast t25
      t27: v8i32 = X86ISD::VSHL t2, t26
              t40: v4i32 = BUILD_VECTOR Constant:i32<32>, Constant:i32<32>, Constant:i32<32>, Constant:i32<32>
            t38: v4i32 = sub t40, t45
          t30: v2i64 = zero_extend_vector_inreg t38
        t31: v4i32 = bitcast t30
      t32: v8i32 = X86ISD::VSRL t2, t31
    t21: v8i32 = or t27, t32
  t10: ch,glue = CopyToReg t0, Register:v8i32 $ymm0, t21
        t4: v8i32,ch = CopyFromReg t0, Register:v8i32 %1
      t6: v8i32 = vector_shuffle<0,0,0,0,0,0,0,0> t4, undef:v8i32
    t43: v4i32 = extract_subvector t6, Constant:i64<0>
    t47: v4i32 = BUILD_VECTOR Constant:i32<31>, Constant:i32<31>, Constant:i32<31>, Constant:i32<31>
  t45: v4i32 = and t43, t47
  t11: ch = X86ISD::RET_FLAG t10, TargetConstant:i32<0>, Register:v8i32 $ymm0, t10:1

Resolved all review comments.

LGTM

This corrects all known problems in the parallel partial sort implementation. The parallel merge algorithm still needs implementing.

Feels a bit like approving myself (which is only partially true) but I don't see any issues with the patch itself.

This change introduced an MSAN error https://bugs.llvm.org/show_bug.cgi?id=50112

Thanks for the test case!
The problem is: