I am quite skeptical how useful this can be in practise. Any motivation examples from real world codes/benchmarks?

Maybe this caused https://bugs.llvm.org/show_bug.cgi?id=50550. Could you check and comment it?

Ok thanks for info. We should follow gcc in this case.

gcc also ignores it?

In D103820#2803120, @davezarzycki wrote:

Actually, wait. Something weird is going on at the mid-level. These two functions should generate the same optimized IR, right?

typedef int V __attribute__((vector_size(64)));

V lt_zero_x_y(V mask, V x, V y) { return mask <  0 ? x : y; }
V ge_zero_y_x(V mask, V x, V y) { return mask >= 0 ? y : x; }

The new results look fine.

In D100581#2798079, @raj.khem wrote:

http://sprunge.us/FJzZXL is a file from harfbuzz and it warns

a.cc:28670:32: error: variable 'supp_size' set but not used [-Werror,-Wunused-but-set-variable]
    unsigned int size0, size1, supp_size = 0;

I do not have -Werror enabled but it still is reported as error. There is no way to disable this warning ?

Thanks!

In D102748#2791077, @xbolva00 wrote:

In D102748#2778813, @xbolva00 wrote:

And there is something weird in terms of cost modelling between vectorizers.

See https://bugs.llvm.org/show_bug.cgi?id=47491

If we dont unroll the loop, llvm loop vectorizer vectorizes the loop (cost model says it is profitable).

If we unroll the loop, slp does not vectorize the unrolled loop.

And other one: https://bugs.llvm.org/show_bug.cgi?id=50552 ... SLP just misses currently these patterns.

Commited without approval?

In D100581#2792854, @Abpostelnicu wrote:

I think there is a false positive with this @george.burgess.iv:
In this we have the warning triggered:
mozglue/baseprofiler/core/platform-linux-android.cpp:216:9: error: variable 'r' set but not used [-Werror,-Wunused-but-set-variable]

SigHandlerCoordinator() {
  PodZero(&mUContext);
  int r = sem_init(&mMessage2, /* pshared */ 0, 0);
  r |= sem_init(&mMessage3, /* pshared */ 0, 0);
  r |= sem_init(&mMessage4, /* pshared */ 0, 0);
  MOZ_ASSERT(r == 0);
}

In D102748#2778813, @xbolva00 wrote:

And there is something weird in terms of cost modelling between vectorizers.

See https://bugs.llvm.org/show_bug.cgi?id=47491

If we dont unroll the loop, llvm loop vectorizer vectorizes the loop (cost model says it is profitable).

If we unroll the loop, slp does not vectorize the unrolled loop.

In D102615#2790544, @mkazantsev wrote:

In D102615#2790190, @xbolva00 wrote:

In D102615#2789636, @mkazantsev wrote:

In D102615#2788964, @xbolva00 wrote:

What are the latest compile time data for this patch?

There have been several separate follow-ups to address the compile time, but I believe we never measured it all together.

Could we get new data? @nikic? I am not sure if followups were not reverted as well so if follow up patches can recommited as NFC(I) patches, it would be great.

The followups were all reverted and integrated into this one. I see no point in merging them separately, unless we want more pain with reverts (if it is needed again). They are just part of the algorithm.

In D102615#2778811, @xbolva00 wrote:

Can this patch optimize code from comment #4 in https://bugs.llvm.org/show_bug.cgi?id=47491 ?

In D102615#2789636, @mkazantsev wrote:

In D102615#2788964, @xbolva00 wrote:

What are the latest compile time data for this patch?

There have been several separate follow-ups to address the compile time, but I believe we never measured it all together.

In D103009#2789489, @nikic wrote:

I think this should check for just LibFunc_malloc and nothing else. MallocLike just means an allocation function that return null on failure, and the nothrow new variants are considered MallocLike and not OpNewLike.

Ok, this looks fine now I think.

What are the latest compile time data for this patch?

In D96223#2788712, @aaron.ballman wrote:

In D96223#2788681, @lebedev.ri wrote:

Reverted in be6b9e8ae71768d2e09ec14619ca4ecfdef553fa - https://godbolt.org/z/3zdqvbfxj
While there, please ensure that other such simplifications don't have similar lingering effects.

Thanks for catching the issue!

FWIW, I think that reverting something several months after it lands is a bit disruptive and the test case causing the revert feels arbitrary without further context that should have been explicitly mentioned in the reverting commit message. The context is that this change broke a test that should have caught the failure: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/misc-static-assert.cpp#L86-L87 which led to this bug report: https://bugs.llvm.org/show_bug.cgi?id=50532. I'm still not convinced the correct action was to revert a change with no discussion that already was shipped in a release, however. Next time, I think it'd be a bit friendlier to mention the regression on the patch to see if the author can do a quick fix -- if no one noticed it for almost four months, it's hard to argue it's a critical issue that needs an immediate revert. (The reversion mildly worries me because this change already shipped as part of Clang 12 and larger reversions make for harder git blame logic to follow along with, both of which can lead to potential confusion for folks.)

Since instcombine is currently only user of emitCalloc, so after you commit this patch, please delete AttrsList parameter from emitCalloc (and adjust this new callsite in DSE) together with removal of this transformation from instcombine.

And remove transformation from instcombine?

In D96418#2783541, @leonardchan wrote:

Well, no, I'm afraid it is actually clear that that code does have UB according to the C++ standard. Perhaps you mean that it *shouldn't* have UB, or that Clang should define its behavior despite the standard.

I might agree with you that I don't see the value in using this stronger rule in C++, but I think it would help to understand the problem a little better. I assume this is causing problems for a less trivial test case? Or do you really have code that's relying on that loop not terminating?

I see. I guess it's good (and bad) that this discovered this UB in our code base. The example I posted is almost exactly what's in our test case. For this particular one, we need a non-terminating thread, so a thread_create function is passed do_nothing. Locally, we could get around this by using something like inline assembly to avoid UB.

(Potentially deviating from this patch) While an infinite loop may be UB according to the standard, it's also something fairly common that can be in a lot of code bases. Although it may be an unintended side-effect, it still seems a bit abrupt that we found about this UB from this patch rather than ubsan or a compiler diagnostic. Are there any plans in the future for something along the lines of improving catching this type of UB? Or alternatively, are there plans of maybe defining it in clang (like a language extension) as @rjmccall points out?

In D102615#2781336, @mkazantsev wrote:

@nikic > Is the "at" part an important part of this optimization?

Unfortunately yes, without it the motivating test won't pass. I'll simply make option to enable/disable this transform.

Would it be fine to revert this for now to work out the kinks?

And there is something weird in terms of cost modelling between vectorizers.

Can this patch optimize code from comment #4 in https://bugs.llvm.org/show_bug.cgi?id=47491 ?

Ah right:) I understand now.

In D28907#2770984, @ABataev wrote:

In D28907#2770982, @SjoerdMeijer wrote:

Hello, this work was brought to my attention in D102748. Would be good to get this in, and seems almost finished too. Any plans to pick this up? Otherwise I think I would be interested in doing that.

This should be much easier to implement after non-power-2 in SLP support.

Re-ping

Also fix a few places where this warning is correctly triggered.

getting this all right and enabled by default with the first commit is probably not achievable

What about C-ray? Does function specialization help?

Great, thanks!

If we improved the robustness of our other optimizations w.r.t. vector instructions, we'd probably be a lot less sensitive to the proposed pass reordering.

This, I think, also relies on the loop vectoriser which seems more powerful than SLP vectorisation currently.

Instead of removing early full unroll pass (see @nikic's concerns), couldn't we run loop vectorizer before full unroll pass?

I don't have suggestion on how to move forward *this* patch, because I think it's fundamentally the wrong direction to take.

Add testcase?

hold off this patch.

If that's implemented in GCC then this change here to build with -fno-semantic-interposition would mean that the test failures seen in D102090 would resurface.

Any testcase to ensure that passes together now produce what you want?

It further requires us to split the current pass into an analysis one and a transformed

+1, agree.

I think we could limit the numbers of function specialized.

Maybe additional info about upper bound (if dst or src is local/global buffer; getDerefenceableBytes API) may be interesting too?

I cannot figure out how to make -fno-semantic-interposition specific to llvm/ and clang/.

I think it would take a long time to tune the heuristics and the cost model may be changing during the evolution process.

absolute numbers being small: do you have any insight into whether the impact is expected to be less on larger benchmarks/binaries?

Manually tagging those arguments with an attribute in exchange for guaranteed specialisation when they're constant would be high value, even without any heuristics to automate the decision.

You dont have to use -Wno-…

Whether we want to run this transformation on newer archs - as you said, it brings us nothing, so atleast we should check it is free in terms of compile time in the backend.

What about IR backward compatibility?

Can you try this patch with http://llvm-compile-time-tracker.com/ ?

What about DeadStoreElimination ? It can also merge stores.. maybe dse needs similar fix.

This could help with https://bugs.llvm.org/show_bug.cgi?id=50105 ?

it seems like we dont specialize std::find to use memchr if possible?

Just curious, why you cannot use:

Hello, some days ago I was looking at it.

Can you collect some performance stats?

• testsuite/spec benchmarks/etc?

In D101840#2737160, @craig.topper wrote:

In D101840#2736994, @xbolva00 wrote:

Moved to InstCombine.

Your test already passes before your change. If the Known bits is negative, PossibleZeros and DefiniteZeros will both be 0 which triggers the PossibleZeros == DefiniteZeros case.

Moved to InstCombine.

// FIXME: This should be in InstSimplify because we're replacing an ...

No strong opinion, but everybody should agree.

Rebased.

Generalization.