Ping

Found trash in the previous change, Update after fix ABataev remarks.

Update after ABataev remarks

Update with solution for the last commit issues with that looks more appropriate to me with just avoiding any vectors like [ operand, operand, op, operand] or [load, load, op, load] in a scheduling bundle, with this solution I have not noticed any bitcode regressions(efficiency) with provided testcase pr34619.ll. I have checked all list of issues from the previous build with this change.

Resolved all issues associated with previous commit.

I think something might be missing here. You're forming a 4x wide load, but you've only proven dereferenceability for offsets 0, 1, 3. (i.e. not 2). How do we know it's safe to dereference between the two elements 1 & 3?

well, it is a maximum reference that was recorded there in InstCombine, that implies 0, 1, 2 and 3.

are you still looking at some other kind of metadata solution? Did you mean for "metadata" here to mean the intrinsic you're proposing?

No, I am happy with intrinsic solution.

Fix review remarks and add an aggregate pointer as intrinsic's second parameter, remove getNextNode() usage.

Fix recordDeadLoad() parameter lining with clang-format.

Fix review remarks and add an aggregate pointer intrinsic's second parameter.

add missed test in the last change.

In D37648#865562, @hfinkel wrote:

Thanks for working on this. Please post an RFC on llvm-dev about the intrinsic and how you intend to use it. You can reference this review, but we should make sure that we have design consensus before proceeding here.

In D37579#869260, @sanjoy wrote:

In D37579#868685, @sanjoy wrote:

Why not leave the original load in place, and tag the load metadata like !speculation_marker ? In either case, this needs a LangRef entry.

Re-reading, I messed up the phrasing there. I meant to say two things:

• You could consider leaving the original load in place, and tag it with something like !speculation_marker. This metadata would indicate to passes like DCE that said load should not be eliminated till the very end of the optimization pipeline (so that we keep the optimization information for as long as possible). This does block optimizations though (like you will not be able to sink stores across this now present load, something you may have been able to do with the intrinsic you're introducing, though not without figuring out a way to denote it as not reading memory), but it has the upside of not introducing a new intrinsic.
• Irrespective of whether you decide to go with the intrinsic or the metadata approach, please also update the language reference with an entry describing the semantics of this new thing.

In D37579#865897, @RKSimon wrote:

Some style comments as the poor bracing/indentation makes it difficult to grok - more thorough review needs to wait until you've addressed @hfinkel's comments on D37648 about phantom mem intrinsics

Update after RKSimon's remarks.

Found typo in previous change. Update.

Rebase

reduce the change a bit further and rebase.

ping

rebase

Rebase

Remove getDefaultConstantForOpcode function for now.

test update to simpler

remove extra flag -slp-vectorizer in test.

update testcase.

update after RKSimon's remarks.

Rebase

correct test filename spelling.

update test.

Update after RKSimon's remarks and replace testcase for more appropriate.

update after RKSimon remarks and add test.

Test cases?

Well, I could add tests here, but it is a like a main flow of this algorithm and it should be tested already here.

Remove technical comments

Fixed issue with horizontal-list.ll.

Oops, I just noticed that we have lost flag in horizontal-list.ll , I will look at the issue.

Rebase

Rebase, remove all "End function" lines.

Reversed back the last change.

update after Craig's remark.

Fix "change to a subregister" issue pointed by Craig.

Update to vector-shuffle-combining-avx.ll

update with X86::AVX512_256_SET0 included, please ignore my last comment

we don't need to change X86::AVX512_256_SET0, it is already using X86::sub_ymm there, I have not seen any ZMM to YMM change in the diff.

ok, thanks, I will redo the change.

Update after RKSimon's remarks.

We decided to split this change into two AVX and AVX512 , this is part one AVX and AVX2.

updated manually vec_uint_to_fp-fastmath.ll, memset.ll

and memset.ll, sorry.

oh, I missed that vec_uint_to_fp-fastmath.ll should be updated manually. I will redo my change.

add X86::AVX512_256_SET0 handling, format, rebase

oh, I missed "X86::AVX512_256_SET0:" case , I have to check some testcases, I think I saw there something incorrect. I will redo my change.

Ping.

rebase after recent changes.

rebase

rebase to the current tree.

update after rksimon's remarks.

Fixed issue with test/Transforms/SLPVectorizer/X86/horizontal-list.ll, Merged two versions of propagateIRFlags into one.

Resulting in to:
VL[0]: %r1 = add i32 %arg, undef
VL[1]: %r2 = add nsw i32 %r1, undef
VL[2]: %r3 = add nsw i32 %r2, undef
VL[3]: %r4 = add nsw i32 %r3, undef
VL[4]: %r5 = add nsw i32 %r4, undef

I'm not sure if Z having those flags is on purpose, though. And I don't see what we get from that.

Please tell me if I misunderstood something. I'm not the most familiar with the SLP vectorizer.

Usually it should have flags in common and it collects those flags see "Intersection->andIRFlags(V);" in the loop across all VL,
but I have one example from test/Transforms/SLPVectorizer/X86/horizontal-list.ll where NUW flag was canceled by NSW.
VL[0]: %r1 = add nuw i32 %arg, undef
VL[1]: %r2 = add nsw i32 %r1, undef
VL[2]: %r3 = add nsw i32 %r2, undef
VL[3]: %r4 = add nsw i32 %r3, undef
VL[4]: %r5 = add nsw i32 %r4, undef

I don't get what you want to do here, sorry.
As I see it, in all the uses of propagateIRFlagsWithOp(X, Y, Z), we have Z == Y[0]. Which will end up being *almost* the same as the propagateIRFlags but with the side-effect that Z will also have those flags.

propagateIRFlagsWithOp only collects flags from Y if Y.Code == Z.Code while propagateIRFlags copies any flag from Y.

Abandoning review due to : there should separate reviews for one for InstCombine Pass and another one for SLP, and dereferenceable_or_null metadata can only be applied to loads of a pointer type.

update after D34756 commit.

update after rksimon's remarks.

Update formatting, comments

Update after http://lists.llvm.org/pipermail/llvm-dev/2017-June/114120.html. Added 0x1b(_CMP_FALSE_OS), 0x1f(_CMP_TRUE_US) handling.

Ping. [andrew.w.kaylor, scanon] Is it OK to assume that FP exceptions are off by default and allow such transformation to constants in the IR since we know that we would have exception with "1.00 -nan" for _mm256_cmp_ps(a, b, 15) in case FP exceptions are enabled?

We should've asked this first: is that fold allowed in the default FPENV state that we assume that clang is operating in?

I suppose it is FE_ALL_EXCEPT.

Abandoning this revision with there is already open https://reviews.llvm.org/D28907 for the issue.

rebased the change against trunk with no new regressions

We agreed with Alexey that I would continue on this issue:
<dinar_> Hi Alexey, I rebased your change for PR30787, do you mind if I update the change in https://reviews.llvm.org/D28907?
<ABataev> Ok, go ahead

Should we handle the 'pd256' version the same way?
How about the 0xb ('false') constant? It should produce a zero here?
Can or should we deal with the signalling versions (0x1b, 0x1f) too?

hm looks like 0xb(_CMP_FALSE_OQ) is ordered, so it is not possible and 0x1b or 0x1f might emit a signal.

add _mm256_cmp_pd double version
add comments in lib/CodeGen/CGBuiltin.cpp
replaced 0xf to _CMP_TRUE_UQ in avx-builtins.c

canceling because x86 is not currently set up to look at breaking a dependency for anything but the 2nd operand.

updated tests after review with proper update_llc_test_checks.py.

Rebased and changed OptForSize to OptForMinSize.