Page MenuHomePhabricator

aqjune (Juneyoung Lee)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 17 2017, 8:49 PM (208 w, 3 d)

Recent Activity

Today

aqjune accepted D94862: [InstCombine] Replace one-use select operand based on condition.

LGTM

Sat, Jan 16, 1:40 PM · Restricted Project
aqjune added a comment to D94860: [InstCombine] Optimize select (X == C), (icmp X Y), false.

The change makes sense..!
Since the patch is more succinct, I'm in favor of applying the patch than this one.

Sat, Jan 16, 3:48 AM · Restricted Project
aqjune added a comment to D94861: [InstCombine,InstSimplify] Optimize select followed by and/or/xor.

After applying D94859 + D94860 + this patch, the diff looks like this:
https://gist.github.com/aqjune/39bcc99bf7339bef74652597c9e57122 (@nikic thanks for creating the _logical functions!)
I think the majority of sound transformations are now supported.

Sat, Jan 16, 3:43 AM · Restricted Project
aqjune requested review of D94861: [InstCombine,InstSimplify] Optimize select followed by and/or/xor.
Sat, Jan 16, 3:31 AM · Restricted Project
aqjune requested review of D94860: [InstCombine] Optimize select (X == C), (icmp X Y), false.
Sat, Jan 16, 2:56 AM · Restricted Project
aqjune committed rGe0a979ccadd8: [InstCombine] Add more tests to select-safe-transforms.ll (NFC) (authored by aqjune).
[InstCombine] Add more tests to select-safe-transforms.ll (NFC)
Sat, Jan 16, 2:49 AM
aqjune committed rGb664bef2ad9a: [InstCombine] Add a test file that contains safe select transforms (NFC) (authored by aqjune).
[InstCombine] Add a test file that contains safe select transforms (NFC)
Sat, Jan 16, 2:28 AM
aqjune added a comment to D94859: [ValueTracking] Make impliesPoison look into operands of V.

I'll leave the diff result after setting -instcombine-unsafe-select-transform=0 at another patch that's going to follow this.

Sat, Jan 16, 1:30 AM · Restricted Project
aqjune requested review of D94859: [ValueTracking] Make impliesPoison look into operands of V.
Sat, Jan 16, 1:29 AM · Restricted Project

Wed, Jan 13

aqjune accepted D94494: canCreateUndefOrPoison: dyn_cast -> dyn_cast_or_null.

LGTM

Wed, Jan 13, 3:48 AM · Restricted Project

Tue, Jan 12

aqjune updated the diff for D90529: Allow nonnull/align attribute to accept poison.

Address comments, leave tests with FIXME

Tue, Jan 12, 7:13 PM · Restricted Project
aqjune committed rG25eb7b08ba77: [DAGCombiner] Fold BRCOND(FREEZE(COND)) to BRCOND(COND) (authored by aqjune).
[DAGCombiner] Fold BRCOND(FREEZE(COND)) to BRCOND(COND)
Tue, Jan 12, 4:58 PM
aqjune closed D92015: [DAGCombiner] Fold BRCOND(FREEZE(COND)) to BRCOND(COND).
Tue, Jan 12, 4:58 PM · Restricted Project
aqjune updated the diff for D92015: [DAGCombiner] Fold BRCOND(FREEZE(COND)) to BRCOND(COND).

Leave a comment about the issue

Tue, Jan 12, 4:57 PM · Restricted Project
aqjune accepted D94550: [InstCombine] Fold select -> and/or using impliesPoison.

LGTM
Yes, I see that such pattern is quite prevalent. I'll make a patch for this.

Tue, Jan 12, 4:49 PM · Restricted Project
aqjune added a comment to D94494: canCreateUndefOrPoison: dyn_cast -> dyn_cast_or_null.

Thanks!
Yes, a test should be added.
I guess

ashr <4 x i16> %induction, select (i1 icmp sgt (i16 ptrtoint (i16* @c to i16), i16 1), <4 x i16> zeroinitializer, <4 x i16> <i16 ptrtoint (i16* @c to i16), i16 ptrtoint (i16* @c to i16), i16 ptrtoint (i16* @c to i16), i16 ptrtoint (i16* @c to i16)>)

is the problematic part?
If it is non-trivial to further reduce the input, you can also add this instruction to the list at ValueTrackingTest.cpp 's TEST(ValueTracking, canCreatePoisonOrUndef).

Tue, Jan 12, 4:44 PM · Restricted Project
aqjune added a comment to D94386: [LangRef] State that a nocapture pointer cannot be returned.

Thank you!

Tue, Jan 12, 4:33 PM · Restricted Project
aqjune added a comment to D92015: [DAGCombiner] Fold BRCOND(FREEZE(COND)) to BRCOND(COND).

Yes, I think this is fine to land as-is. Note that SDAG always works on a single basic block, so I don't think there is any value in having "branch on undef is UB" semantics, as we can't infer information from branch conditions anyway.

Tue, Jan 12, 4:32 PM · Restricted Project
aqjune committed rG76643c48cddd: [LangRef] State that a nocapture pointer cannot be returned (authored by aqjune).
[LangRef] State that a nocapture pointer cannot be returned
Tue, Jan 12, 4:31 PM
aqjune closed D94386: [LangRef] State that a nocapture pointer cannot be returned.
Tue, Jan 12, 4:31 PM · Restricted Project
aqjune added a comment to D90529: Allow nonnull/align attribute to accept poison.

@jdoerfert Would it be okay if I land this after a week? D91480 also uses the semantics described in this patch as well.

Tue, Jan 12, 4:56 AM · Restricted Project
aqjune added a comment to D94386: [LangRef] State that a nocapture pointer cannot be returned.

Should we add the, "if broken -> UB!" clause while we are here?

Tue, Jan 12, 4:53 AM · Restricted Project

Mon, Jan 11

aqjune added a comment to D93065: [InstCombine] Disable optimizations of select instructions that causes propagation of poison values.

@aqjune Do you have any automated way of duplicating the InstCombine tests with logical and/or, similar to what you did for insertelement? Or does this have to be done by hand?

Mon, Jan 11, 4:40 AM · Restricted Project, Restricted Project

Sun, Jan 10

aqjune requested review of D94386: [LangRef] State that a nocapture pointer cannot be returned.
Sun, Jan 10, 9:44 PM · Restricted Project
aqjune added inline comments to D93817: [InstCombine] Update transformations to use poison for insertelement/shufflevector's placeholder value (2/2).
Sun, Jan 10, 7:18 PM · Restricted Project, Restricted Project
aqjune updated the diff for D93817: [InstCombine] Update transformations to use poison for insertelement/shufflevector's placeholder value (2/2).

Update

Sun, Jan 10, 7:09 PM · Restricted Project, Restricted Project
aqjune retitled D93817: [InstCombine] Update transformations to use poison for insertelement/shufflevector's placeholder value (2/2) from Update transformations to use poison for insertelement/shufflevector's placeholder value to [InstCombine] Update transformations to use poison for insertelement/shufflevector's placeholder value (2/2).
Sun, Jan 10, 6:57 PM · Restricted Project, Restricted Project
aqjune updated the diff for D93817: [InstCombine] Update transformations to use poison for insertelement/shufflevector's placeholder value (2/2).

Leave updates in InstCombineVectorOps.cpp only

Sun, Jan 10, 6:55 PM · Restricted Project, Restricted Project
aqjune added inline comments to D94380: [InstCombine] Update transformations to use poison for insertelement/shufflevector's placeholder value (1/2).
Sun, Jan 10, 5:33 PM · Restricted Project, Restricted Project
aqjune requested review of D94380: [InstCombine] Update transformations to use poison for insertelement/shufflevector's placeholder value (1/2).
Sun, Jan 10, 5:28 PM · Restricted Project, Restricted Project
aqjune committed rG9f2d9364b04c: [CodeGen] Update transformations to use poison for shufflevector/insertelem's… (authored by aqjune).
[CodeGen] Update transformations to use poison for shufflevector/insertelem's…
Sun, Jan 10, 1:06 AM
aqjune closed D94056: [CodeGen] Update transformations to use poison for shufflevector/insertelem's initial vector elem.
Sun, Jan 10, 1:06 AM · Restricted Project
aqjune updated the diff for D94056: [CodeGen] Update transformations to use poison for shufflevector/insertelem's initial vector elem.

Address comments

Sun, Jan 10, 1:04 AM · Restricted Project

Thu, Jan 7

aqjune added inline comments to D93189: Introduce the `!nocapture` metadata and "nocapture_use" operand bundle.
Thu, Jan 7, 9:44 PM · Restricted Project
aqjune added a comment to D94002: [LangRef] Make lifetime intrinsic's semantics consistent with StackColoring's comment.

Thinking about @jdoerfert 's suggestion again, it still implies that objects may have overlapping addresses regardless of lifetime. Am I understanding correctly?

Thu, Jan 7, 9:24 PM · Restricted Project
aqjune updated the diff for D94002: [LangRef] Make lifetime intrinsic's semantics consistent with StackColoring's comment.

It is memset(undef) in other cases

Thu, Jan 7, 9:19 PM · Restricted Project
aqjune added a comment to D92270: [ConstantFold] Fold more operations to poison.

It turned out to be UB in our code as far as I can tell, see https://bugs.chromium.org/p/angleproject/issues/detail?id=5500#c36 and the follow-on.

(If this is known to trigger an existing bug more often, it might still be a good idea to undo it until that existing bug is fixed? But we're not affected by this existing bug as far as I can tell, so this is just a suggestion.)

Thu, Jan 7, 8:58 PM · Restricted Project, Restricted Project
aqjune added a comment to D93189: Introduce the `!nocapture` metadata and "nocapture_use" operand bundle.

In other words, is it purely for making analysis flow-insensitive?

Thu, Jan 7, 4:46 PM · Restricted Project
aqjune added a comment to D93189: Introduce the `!nocapture` metadata and "nocapture_use" operand bundle.

I have a question - is it possible to have the same optimization power with !nocapture only? In other words, is it purely for making analysis flow-insensitive?

Thu, Jan 7, 4:45 PM · Restricted Project

Wed, Jan 6

aqjune added a comment to D90529: Allow nonnull/align attribute to accept poison.

@efriedma ping

Wed, Jan 6, 6:25 PM · Restricted Project
aqjune added a comment to D94180: [SimplifyCFG] Optimize CFG when null is passed to a function with nonnull argument.

Yeah, I believe passing null to nonnull should not immediately raise UB; it will block useful analyses. The patch is D90529, and I need to push it... Maybe it is time to reduce the number of patches that are still open by me.

Wed, Jan 6, 6:25 PM · Restricted Project
aqjune added inline comments to D93994: [InstSimplify] Fold insertelement vec, poison, idx into vec.
Wed, Jan 6, 5:17 PM · Restricted Project
aqjune committed rG3a60a1f16570: [InstSimplify] Fold insertelement vec, poison, idx into vec (authored by aqjune).
[InstSimplify] Fold insertelement vec, poison, idx into vec
Wed, Jan 6, 5:10 PM
aqjune closed D93994: [InstSimplify] Fold insertelement vec, poison, idx into vec.
Wed, Jan 6, 5:10 PM · Restricted Project
aqjune committed rGc95f39891a28: [Constant] Add tests for ConstantVector::get (NFC) (authored by aqjune).
[Constant] Add tests for ConstantVector::get (NFC)
Wed, Jan 6, 5:08 PM
aqjune committed rG8871a4b4cab8: [Constant] Update ConstantVector::get to return poison if all input elems are… (authored by aqjune).
[Constant] Update ConstantVector::get to return poison if all input elems are…
Wed, Jan 6, 4:26 PM
aqjune added a comment to D92270: [ConstantFold] Fold more operations to poison.

To fix the old bug quite a few patches in LLVM have landed so far and it is still ongoing.

Wed, Jan 6, 4:03 PM · Restricted Project, Restricted Project
aqjune added a comment to D92270: [ConstantFold] Fold more operations to poison.

We bisected a test failure to this (https://bugs.chromium.org/p/angleproject/issues/detail?id=5500#c17). Can you expand a bit on what this patch means in practice? I'm guessing it makes UB in C++ code have bad effects more often? If so, what type of UB?

Wed, Jan 6, 4:02 PM · Restricted Project, Restricted Project

Tue, Jan 5

aqjune updated the diff for D93994: [InstSimplify] Fold insertelement vec, poison, idx into vec.

Resolve failures
A few of those was regressions. It was because ConstantVector::get(Arr) was unable to understand Poison array. I updated those.

Tue, Jan 5, 8:26 PM · Restricted Project
aqjune added inline comments to D94056: [CodeGen] Update transformations to use poison for shufflevector/insertelem's initial vector elem.
Tue, Jan 5, 8:11 PM · Restricted Project
aqjune added a comment to D93943: [SimplifyCFG] Update SimplifyBranchOnICmpChain to recognize select form of and/or.

Thinking about this again, don't we need to always freeze the extra condition (unless known non-undef/poison)? While poison is only a problem when converting the select form into a branch, doesn't undef need to be frozen even in the and/or form? As the other operand may make it unconditionally true/false and thus well-defined.

In that case, I think we should either always insert the freeze, or not inserted it in the select form either (your original patch) with a FIXME for the more general problem.

Tue, Jan 5, 7:19 PM · Restricted Project
aqjune accepted D93998: [InstSimplify] Fold out-of-bounds shift to poison.

LGTM

Tue, Jan 5, 7:14 PM · Restricted Project
aqjune committed rG29f8628d1fc8: [Constant] Add containsPoisonElement (authored by aqjune).
[Constant] Add containsPoisonElement
Tue, Jan 5, 7:12 PM
aqjune closed D94053: [Constant] Add containsPoisonElement.
Tue, Jan 5, 7:12 PM · Restricted Project
aqjune updated the diff for D94053: [Constant] Add containsPoisonElement.

Use function_ref

Tue, Jan 5, 7:11 PM · Restricted Project
aqjune added a comment to D93994: [InstSimplify] Fold insertelement vec, poison, idx into vec.

Oh yes, I'll look into them

Tue, Jan 5, 6:50 PM · Restricted Project
aqjune committed rG8444a2494d3d: [X86] Update X86InstCombineIntrinsic to use CreateShuffleVector with one vector (authored by aqjune).
[X86] Update X86InstCombineIntrinsic to use CreateShuffleVector with one vector
Tue, Jan 5, 6:43 PM
aqjune closed D94059: [X86] Update X86InstCombineIntrinsic to use CreateShuffleVector with one vector.
Tue, Jan 5, 6:43 PM · Restricted Project
aqjune updated the diff for D94002: [LangRef] Make lifetime intrinsic's semantics consistent with StackColoring's comment.

Address comments

Tue, Jan 5, 6:40 PM · Restricted Project
aqjune added a comment to D94061: [SLP,LV] Use poison constant vector for shufflevector/initial insertelement.

LGTM, as this is aligned with the overall direction we are going.

Tue, Jan 5, 6:23 PM · Restricted Project
aqjune committed rG4a8e6ed2f795: [SLP,LV] Use poison constant vector for shufflevector/initial insertelement (authored by aqjune).
[SLP,LV] Use poison constant vector for shufflevector/initial insertelement
Tue, Jan 5, 6:23 PM
aqjune closed D94061: [SLP,LV] Use poison constant vector for shufflevector/initial insertelement.
Tue, Jan 5, 6:23 PM · Restricted Project
aqjune added inline comments to D94002: [LangRef] Make lifetime intrinsic's semantics consistent with StackColoring's comment.
Tue, Jan 5, 6:15 PM · Restricted Project
aqjune added inline comments to D94002: [LangRef] Make lifetime intrinsic's semantics consistent with StackColoring's comment.
Tue, Jan 5, 6:11 PM · Restricted Project
aqjune updated the diff for D94059: [X86] Update X86InstCombineIntrinsic to use CreateShuffleVector with one vector.

Fixed, thanks :)

Tue, Jan 5, 4:41 AM · Restricted Project

Mon, Jan 4

aqjune updated the diff for D94061: [SLP,LV] Use poison constant vector for shufflevector/initial insertelement.

Updates

Mon, Jan 4, 10:06 PM · Restricted Project
aqjune updated the diff for D93817: [InstCombine] Update transformations to use poison for insertelement/shufflevector's placeholder value (2/2).

Rebase

Mon, Jan 4, 9:46 PM · Restricted Project, Restricted Project
aqjune requested review of D94061: [SLP,LV] Use poison constant vector for shufflevector/initial insertelement.
Mon, Jan 4, 9:18 PM · Restricted Project
aqjune retitled D94059: [X86] Update X86InstCombineIntrinsic to use CreateShuffleVector with one vector from [X86] Update X86InstCombineIntrinsic to use unary CreateShuffleVector to [X86] Update X86InstCombineIntrinsic to use CreateShuffleVector with one vector.
Mon, Jan 4, 8:41 PM · Restricted Project
aqjune requested review of D94059: [X86] Update X86InstCombineIntrinsic to use CreateShuffleVector with one vector.
Mon, Jan 4, 8:40 PM · Restricted Project
aqjune requested review of D94056: [CodeGen] Update transformations to use poison for shufflevector/insertelem's initial vector elem.
Mon, Jan 4, 7:39 PM · Restricted Project
aqjune added inline comments to D93998: [InstSimplify] Fold out-of-bounds shift to poison.
Mon, Jan 4, 6:10 PM · Restricted Project
aqjune committed rGf665a8c5b8b4: [InstSimplify] gep with poison operand is poison (authored by aqjune).
[InstSimplify] gep with poison operand is poison
Mon, Jan 4, 6:09 PM
aqjune committed rGf28b026d32ca: [InstSimplify] add a test for gep with poison operand (NFC) (authored by aqjune).
[InstSimplify] add a test for gep with poison operand (NFC)
Mon, Jan 4, 6:03 PM
aqjune added inline comments to D94002: [LangRef] Make lifetime intrinsic's semantics consistent with StackColoring's comment.
Mon, Jan 4, 5:53 PM · Restricted Project
aqjune added inline comments to D93998: [InstSimplify] Fold out-of-bounds shift to poison.
Mon, Jan 4, 5:28 PM · Restricted Project
aqjune added inline comments to D93994: [InstSimplify] Fold insertelement vec, poison, idx into vec.
Mon, Jan 4, 5:21 PM · Restricted Project
aqjune updated the diff for D93994: [InstSimplify] Fold insertelement vec, poison, idx into vec.

Address comment

Mon, Jan 4, 5:20 PM · Restricted Project
aqjune requested review of D94053: [Constant] Add containsPoisonElement.
Mon, Jan 4, 5:18 PM · Restricted Project
aqjune committed rGabbef2fd46d4: [ValueTracking] isGuaranteedNotToBePoison should return true on undef (authored by aqjune).
[ValueTracking] isGuaranteedNotToBePoison should return true on undef
Mon, Jan 4, 1:50 PM

Sun, Jan 3

aqjune added a comment to D93376: [LangRef] Clarify the semantics of lifetime intrinsics.

The first patch is here: D94002

Sun, Jan 3, 5:15 PM · Restricted Project
aqjune updated the diff for D94002: [LangRef] Make lifetime intrinsic's semantics consistent with StackColoring's comment.

Minor update

Sun, Jan 3, 5:12 PM · Restricted Project
aqjune added inline comments to D94002: [LangRef] Make lifetime intrinsic's semantics consistent with StackColoring's comment.
Sun, Jan 3, 5:11 PM · Restricted Project
aqjune requested review of D94002: [LangRef] Make lifetime intrinsic's semantics consistent with StackColoring's comment.
Sun, Jan 3, 5:07 PM · Restricted Project
aqjune accepted D93995: [InstSimplify] Fold division by zero to poison.

LGTM - Thanks!

Sun, Jan 3, 11:51 AM · Restricted Project
aqjune committed rG49c2d703d344: [X86] Make deinterleave8bitStride3 use unary CreateShuffleVector (authored by aqjune).
[X86] Make deinterleave8bitStride3 use unary CreateShuffleVector
Sun, Jan 3, 9:14 AM
aqjune closed D93993: [X86] Make deinterleave8bitStride3 use unary CreateShuffleVector.
Sun, Jan 3, 9:14 AM · Restricted Project
aqjune added a comment to D93993: [X86] Make deinterleave8bitStride3 use unary CreateShuffleVector.

LGTM. You might want to update the comment at the top of the file to use poison instead of undef (just to show the canonical IR, doesn't matter semantically).

Sun, Jan 3, 9:08 AM · Restricted Project
aqjune requested review of D93994: [InstSimplify] Fold insertelement vec, poison, idx into vec.
Sun, Jan 3, 9:05 AM · Restricted Project
aqjune updated the diff for D93817: [InstCombine] Update transformations to use poison for insertelement/shufflevector's placeholder value (2/2).

Rebase
I'll continue splitting after working on lifetime patches

Sun, Jan 3, 8:43 AM · Restricted Project, Restricted Project
aqjune added inline comments to D93990: [InstSimplify] Return poison if insertelement touches out of bounds.
Sun, Jan 3, 8:18 AM · Restricted Project
aqjune requested review of D93993: [X86] Make deinterleave8bitStride3 use unary CreateShuffleVector.
Sun, Jan 3, 8:16 AM · Restricted Project
aqjune committed rG2139958b5344: [InstSimplify] Return poison if insertelement touches out of bounds (authored by aqjune).
[InstSimplify] Return poison if insertelement touches out of bounds
Sun, Jan 3, 7:43 AM
aqjune closed D93990: [InstSimplify] Return poison if insertelement touches out of bounds.
Sun, Jan 3, 7:43 AM · Restricted Project
aqjune committed rG1fc992bd864a: [Scalarizer] Use poison as insertelement's placeholder (authored by aqjune).
[Scalarizer] Use poison as insertelement's placeholder
Sun, Jan 3, 7:36 AM
aqjune closed D93989: [Scalarizer] Use poison as insertelement's placeholder.
Sun, Jan 3, 7:36 AM · Restricted Project
aqjune added a comment to D93065: [InstCombine] Disable optimizations of select instructions that causes propagation of poison values.

For loop unswitch, I'll work on it after D93764 is done

Sun, Jan 3, 7:34 AM · Restricted Project, Restricted Project
aqjune added a comment to D93882: [SCEV] recognize logical and/or pattern.

Something that crossed my mind: While we now handle logical and/or correctly, the same underlying problem also exists for multi-exit loops. In that case we'd combine exit limits from multiple branches via umin(), even though some of them may be poisonous.

Sun, Jan 3, 7:15 AM · Restricted Project
aqjune requested review of D93990: [InstSimplify] Return poison if insertelement touches out of bounds.
Sun, Jan 3, 7:06 AM · Restricted Project
aqjune requested review of D93989: [Scalarizer] Use poison as insertelement's placeholder.
Sun, Jan 3, 6:18 AM · Restricted Project