This is an archive of the discontinued LLVM Phabricator instance.

[CodeGenPrepare] Add BothExtension type to PromotedInsts
ClosedPublic

Authored by Carrot on Jul 18 2018, 2:49 PM.

Details

Summary

This patch fixes PR38125.

Instruction extension types are recorded in PromotedInsts, it can be used later in function canGetThrough. If an instruction has two users with different extension types, it will be inserted into PromotedInsts two times in function promoteOperandForOther. The second one overwrites the first one, and the final extension type is wrong, later causes problem in canGetThrough.

This patch changes the simple bool extension type to 2-bit enum type, add a BothExtension type in addition to zero/sign extension. When an user sees BothExtension for an instruction, it actually knows nothing about how that instruction is extended.

Diff Detail

Repository
rL LLVM

Event Timeline

Carrot created this revision.Jul 18 2018, 2:49 PM

from someone who is just an observer: What happens when we see IsSExt types

A
B
A

?
Then the last one will re-insert A, which is probably not what you want.

from someone who is just an observer: What happens when we see IsSExt types

A
B
A

?
Then the last one will re-insert A, which is probably not what you want.

Good catch!
So we should record zero/sign/both extension types of an instruction instead of delete it.

Carrot updated this revision to Diff 156376.Jul 19 2018, 3:34 PM
Carrot retitled this revision from [CodeGenPrepare] Remove instruction from PromotedInsts if we have seen it multi times to [CodeGenPrepare] Add BothExtension type to PromotedInsts.
Carrot edited the summary of this revision. (Show Details)
Carrot added a comment.Aug 3 2018, 4:28 PM

friendly ping.

Shouldn't we keep only the latest type and kind of extension?

To be more precise, the latest promotion should be correct, but if we want the full information of multiple extension, the current patch is I believe wrong.
E.g., zext (sext i8 to i16) to i32, doesn't give you the same warranties on the bit pattern than what zext i8 to i32 gives, whereas we won't be able to see that.

To be more precise, the latest promotion should be correct, but if we want the full information of multiple extension, the current patch is I believe wrong.
E.g., zext (sext i8 to i16) to i32, doesn't give you the same warranties on the bit pattern than what zext i8 to i32 gives, whereas we won't be able to see that.

qcolombet added inline comments.Aug 6 2018, 5:17 PM
lib/CodeGen/CodeGenPrepare.cpp
3600 ↗(On Diff #156376)

Instead of insert, we should do update the value all the time.

Carrot added a comment.Aug 7 2018, 3:28 PM

Shouldn't we keep only the latest type and kind of extension?

We should keep only the latest committed type and kind of extension, without reverted extension.

Carrot added a comment.Aug 7 2018, 4:03 PM

To be more precise, the latest promotion should be correct, but if we want the full information of multiple extension, the current patch is I believe wrong.
E.g., zext (sext i8 to i16) to i32, doesn't give you the same warranties on the bit pattern than what zext i8 to i32 gives, whereas we won't be able to see that.

To be even more precise, the latest committed promotions should be correct, reverted promotions are not correct.

This patch does not use the full information of multiple extensions, it just marked the fact, and later query the instruction through getOrigType, BothExtension can't match any actual extension type, so nullptr is returned.

So this is a quick but conservative patch.

lib/CodeGen/CodeGenPrepare.cpp
3600 ↗(On Diff #156376)

Function addPromotedInst exactly does this.

qcolombet added inline comments.Aug 8 2018, 2:46 PM
lib/CodeGen/CodeGenPrepare.cpp
3295 ↗(On Diff #156376)

When the type of the extension doesn't change, we should avoid updating the type, as the source type gives more information and is still relevant.

3307 ↗(On Diff #156376)

I missed that part on my first read. Add a comment saying that if the extension doesn't match, we cannot use the information we had on the original type.

3600 ↗(On Diff #156376)

Oh sorry.

I missed it the first time, as I missed there was a new getOrigType function.
One thing that could help is a better documentation/name for the enum, e.g., instead of both something like stacked or instead of we saw both extension something that says one after the other.

test/Transforms/CodeGenPrepare/X86/pr38125.ll
1 ↗(On Diff #156376)

Please use a name for the filename that conveys what this test is checking without to have to check the PR.

31 ↗(On Diff #156376)

Can't this be simplified further?
I don't think we would need more than one block for the problem at stake, right?

Carrot updated this revision to Diff 160142.Aug 10 2018, 10:43 AM
Carrot marked 5 inline comments as done.
qcolombet accepted this revision.Aug 13 2018, 9:28 AM

Thanks for your patience.
LGTM.
One final nit below. Feel free to commit directly.

test/Transforms/CodeGenPrepare/X86/multi-extension.ll
11 ↗(On Diff #160142)

Add a comment highlighting the characteristic of the test.
I.e., %or is reachable by both a sext and zext that are going to be promoted.

This revision is now accepted and ready to land.Aug 13 2018, 9:28 AM
Closed by commit rL339822 (authored by Carrot). · Explain WhyAug 15 2018, 3:09 PM
This revision was automatically updated to reflect the committed changes.