Page MenuHomePhabricator

[InstCombine] convert assumes to operand bundles
Needs ReviewPublic

Authored by Tyker on Jun 27 2020, 4:23 AM.

Details

Summary

Instcombine will convert the nonnull and alignment assumption that use the boolean condtion
to an assumption that uses the operand bundles when knowledge retention is enabled.

Diff Detail

Event Timeline

Tyker created this revision.Jun 27 2020, 4:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2020, 4:23 AM
Tyker updated this revision to Diff 273897.Jun 27 2020, 4:28 AM

there is an NFC change to the test that was excluded from the diff to make it easier to read.

Initial comments.

llvm/include/llvm/Transforms/Utils/AssumeBundleBuilder.h
45

Please add some documentation here.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
1487

I don't get this. Doesn't it replace the operand of an assume with false if there is one with the same condition following? That seems wrong, maybe "true" is what you wanted?

1510

Comments on what is happening here and below please.

1522

Isn't this call the same as the 5 lines before?

llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp
308

It is odd that this takes an insertion point but apparently does not insert the instruction. Can we change either to confuse me less?

llvm/test/Transforms/InstCombine/assume.ll
152

Why do we have SAME checking the function twice? Is this a bug in the update script?

Tyker updated this revision to Diff 275501.Jul 4 2020, 7:42 AM
Tyker marked 6 inline comments as done.

addressed the comments
the previous patch had many stupid mistakes

llvm/test/Transforms/InstCombine/assume.ll
152

Is this a bug in the update script?

perhaps but after purging every CHECK line the issue is gone.

Looks conceptually good but there are some oddities, potentially in the lookup.

llvm/test/Transforms/InstCombine/assume.ll
52

This doesn't seem to work properly.

@lebedev.ri found other problematic cases. One that I now know of is multiple bundles: https://godbolt.org/z/To5obz

302

Can u add a FIXME or NOTE here to indicate we could duplicate the load instead and keep a assume with ["nonnull"(%load)] around.

493

Again a fold missing CMP = true

Tyker updated this revision to Diff 276824.Jul 9 2020, 1:22 PM

I had to merge the previous patch with other i had laying around and add some more
to make the bundle mode be at least as good as the default. so this revision got
quite a bit bigger.

I hope its still fine.

jdoerfert added inline comments.Aug 4 2020, 5:37 PM
llvm/include/llvm/Analysis/AssumeBundleQueries.h
111

This is not a strict order. Let's use AttrKind and WasOn as well.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
93

Add a sentence what this controls/means.

1472

I'd add a message to the assert and a comment before the lambda.

1547

Shouldn't we build a GEP for the offset instead, if the offset is not a multiple of the alignment that is.

Tyker updated this revision to Diff 283996.Aug 7 2020, 12:46 PM
Tyker marked 3 inline comments as done.

addressed comments

jdoerfert added inline comments.Aug 10 2020, 11:16 AM
llvm/include/llvm/Analysis/AssumeBundleQueries.h
114

Nit: Add a message to the assert please explaining what usage is allowed.

llvm/lib/Analysis/Loads.cpp
94

Nit: Add a comment describe what is happening here.

I guess we should have a todo that states partial information, e.g., only AlignRK or DerefRK, should be used in other places too. The entire function should be rewritten for that so it is out of scope.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
1547

At least a TODO would be good.

Tyker updated this revision to Diff 287209.Aug 22 2020, 11:02 AM
Tyker marked an inline comment as done.

Addressed comments.

Tyker updated this revision to Diff 294500.Sep 26 2020, 7:00 AM

rebased
add cannonicalization of assume bundles

jdoerfert added inline comments.
llvm/test/Transforms/InstCombine/assume.ll
258

It is questionable if we should prefer the assume or the !nonnull metadata here.

695

Also unclear if this is better than the GEP. I guess the case with an offset of 4 is clear but this one I don't know.

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

Tyker updated this revision to Diff 317164.Sat, Jan 16, 3:38 AM

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

I can do the upstreaming but i don't know what the current state is for upstreaming this.

llvm/test/Transforms/InstCombine/assume.ll
695

this is one of the default canonicalization done when building assumes maybe we should keep both.
the idea behind it is to make the knowledge usable by all derived pointers and also minimize the number of assumes but i agree it is not clear its better.

Tyker added inline comments.Sat, Jan 16, 3:39 AM
llvm/test/Transforms/InstCombine/assume.ll
258

fixed

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

I can do the upstreaming but i don't know what the current state is for upstreaming this.

I think there are different things in this patch, some can be upstreamed some need minor adjustment, and some I don't understand yet. See my comments. It would be great if we can finish this line of assume optimizations though.

llvm/include/llvm/Transforms/Utils/AssumeBundleBuilder.h
67

Nit: The first sentence is hard to read.

llvm/lib/Analysis/ValueTracking.cpp
756

Why can we allow ephemeral here without potentially throwing away an assume that was used to argue it itself is useless? Should we not do the AllowEphemeral stuff for now?

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
1474

Nit: Typo, arguement

1578

Nit: typo: help

1585

losses what?

1593

I'm not sure why we return nullptr here and not continue going.

llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp
315

!Assume means Assume->GetModule() will crash, probably add a DL argument.

llvm/test/Analysis/ValueTracking/assume.ll
75

How do we get an alignment of 8 here?