LGTM

seems good to me. but maybe other still have comments.

In D98228#2624345, @aqjune wrote:

In D98228#2613304, @Tyker wrote:

I think the process of checking is a value is noundef needs to be more general than just calls. maybe addKnowledge would be a better place.
because nonnull and align information can also be inferred from loads/stores.

Hi, sorry for my delay in response.
Could you elaborate a bit with an example, please?
Does it mean that the updated analysis may return suboptimal results?

In D90529#2619492, @aqjune wrote:

In D90529#2613303, @Tyker wrote:

In D90529#2612794, @aqjune wrote:

A fix in AssumeBundleBuilder to make it comply LangRef: D98228

BTW, I found that "align" can take two operands: "align"(i8* ptr, i64 a, i64 b) What is the meaning of the second index (b)?

the meaning of the second index is the offset of the alignment. so with "align"(i8* ptr, i64 16, i64 12), ptr is aligned on 4 but ptr + 4 is aligned on 16.
I introduced this to support __builtin_assume_aligned which has similar semantics.

Hi, thanks for the info.

But I think an example in Transforms/AlignmentFromAssumptions/simple.ll conflicts with your definition. It has:

28 define i32 @foo2a(i32* nocapture %a) nounwind uwtable readonly {
29 entry:
30   tail call void @llvm.assume(i1 true) ["align"(i32* %a, i32 32, i32 28)]
31   %arrayidx = getelementptr inbounds i32, i32* %a, i64 -1
32   %0 = load i32, i32* %arrayidx, align 4
33   ret i32 %0
34 
35 ; CHECK-LABEL: @foo2a
36 ; CHECK: load i32, i32* {{[^,]+}}, align 32
37 ; CHECK: ret i32
38 }

"align"(i32* %a, i32 32, i32 28) means %a + 4 is 32-bytes aligned, IIUC. Then, %a - 4 cannot be 32-bytes aligned, is it?

Yes, this looks like a bug.

I think the process of checking is a value is noundef needs to be more general than just calls. maybe addKnowledge would be a better place.
because nonnull and align information can also be inferred from loads/stores.

In D90529#2612794, @aqjune wrote:

A fix in AssumeBundleBuilder to make it comply LangRef: D98228

BTW, I found that "align" can take two operands: "align"(i8* ptr, i64 a, i64 b) What is the meaning of the second index (b)?

In D96646#2594843, @andrew.w.kaylor wrote:

I'm not trying to be difficult, but I genuinely still don't understand the additional arguments pointer. Is it intended to allow proprietary extensions? Is there an example somewhere?

If these intrinsics are meant as a general mechanism to enable arbitrary communication between custom front ends and custom optimization passes, that's fine. I'd just like to see something explicitly explaining that.

added more details about the storage of additional arguments.

fixed the sentence.

addressed comment

This seems good to me but i am not familiar the the auto-upgrader.

I added the changes to the langref in https://reviews.llvm.org/D96646

rebased, addressed comments. fixed issues.

In D82703#2489196, @jdoerfert wrote:

@Tyker are you going to upstream this? If not, can someone take over?

In rGd093401a2617d3c46aaed9eeaecf877e3ae1a9f1#958951, @aaron.ballman wrote:

Just an FYI, but this probably should have gotten a review as it's not NFC (it changes what information gets dumped in text and JSON form, impacts AST matching behavior, and other minor effects). That said, overall the direction seems fine to me, though I did question one of the changes.

I recently made it much easier to create AnnotationAttr in this context with https://reviews.llvm.org/rGd093401a2617d3c46aaed9eeaecf877e3ae1a9f1.

In D88645#2353152, @thakis wrote:

Looks like this broke tests: http://45.33.8.238/linux/31159/step_12.txt

Please take a look, and revert for now if it takes a while to fix.

this is fixed by 4afa077899b

In D88645#2347731, @aaron.ballman wrote:

In D88645#2347725, @Tyker wrote:

In D88645#2347050, @aaron.ballman wrote:

LGTM aside from a request for a comment to be added. Thank you!

do you mean an RFC on llvm-dev/cfe-dev ?

Oh gosh no! I just meant I was asking for a comment to be added in SemaDeclAttr.td before you commit. Sorry for the confusion! :-)

In D88645#2347050, @aaron.ballman wrote:

LGTM aside from a request for a comment to be added. Thank you!

In D63640#2339917, @rsmith wrote:

In D63640#2331779, @Tyker wrote:

but the "real" blocker is that the testing depends on D85144 for testing.
we could land it marking the tests XFAIL and correct it when dumping improvements arrive.

I'd be OK with that. We'll have coverage for this in various forms landing pretty soon.

addressed comments.

addressed comments.

rebased and add more fixes

In D63640#2331734, @martong wrote:

In D63640#2331410, @rsmith wrote:

Reverse ping: I have a patch implementing class type non-type template parameters

that's blocked on this landing. If you won't have time to address @martong's comments soon, do you mind if I take this over and land it?

try to apply martongs's suggestions.

What about leaving call llvm.assume whenever the load !noundef is moved to somewhere else? I guess this is what LLVM is doing as well.

If --enable-knowledge-retention is set, we should certainly do that if we would otherwise strip the metadata (not only for noundef).

this is not implemented the assume building currently don't look at any metadata.

you probably want to add noundef to isUsefullToPreserve such that it gets preserved automatically when knowledge retention is turned on.

addressed comments.

addressed comments.

This change makes a lot of sense, but i don't understand all the consequences either.

addressed comments

rebased
add cannonicalization of assume bundles

this was a mistake i intended to update D82703

In D86816#2273029, @jdoerfert wrote:

OK, I think we need to revisit assume but an otherwise empty loop with an assume doesn't seem to be particularly useful. We might want to hoist loop invariant assumes, but that can be revisited. I think this solves a problem and is reasonable until we have time and ideas.

rebased

In D86816#2245470, @jdoerfert wrote:

I'd almost say mayHaveSideEffects could have a boolean parameter to ignore droppable uses. Either way, this is right now the best we can do, teach the things that query mayHaveSideEffects and friends about droppable uses. If we have them as flags to all of them that might be easier to do.

yeah linkage was already changed.
added a test

In D86404#2232128, @lebedev.ri wrote:

Needs two tests - i agree that comdat makes sense, but i'm not sure about linkage.

Addressed comments.

In D85996#2219650, @lebedev.ri wrote:

This seems fine to me, thanks.
If you have llvm-reduce crashers, do let me know, i'm interested.

In D85996#2219124, @lebedev.ri wrote:

1. I'm pretty sure it's not okay to allocate memory in crash handler.
2. We can't know that the internal state is still consistent.

i agree with both points but all i was trying to do i salvage what can be salvaged.

In D83507#2215586, @lebedev.ri wrote:

Please rebase and update the affected test :)

In D85144#2205461, @riccibruno wrote:

I agree with you that it's fine to use printPretty for leaves (and additionally it would be annoying to duplicate the LValue case); that's what I was planning to do anyway.

What I am not sure I agree with is the additional complexity to handle the (debugger-only and easy to avoid) case where no context is given.

thanks for this,

addressed comments

rebased

this a patch i had to improve dumping before your improvement to dumping.

remove unintended code.