Page MenuHomePhabricator

[InstCombine] convert assumes to operand bundles
Needs ReviewPublic

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

Details

Reviewers
jdoerfert
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
4247

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?

4268

Comments on what is happening here and below please.

4280

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

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

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
192–193

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
192–193

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
56

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

362

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

574

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
94

Add a sentence what this controls/means.

4231

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

4305

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
4305

At least a TODO would be good.

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

Addressed comments.