Page MenuHomePhabricator

aqjune (Juneyoung Lee)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 17 2017, 8:49 PM (186 w, 2 d)

Recent Activity

Yesterday

aqjune updated the diff for D84941: [JumpThreading] Let ProcessImpliedCondition look into freeze instructions.

Rebase

Thu, Aug 13, 6:44 PM · Restricted Project
aqjune updated the summary of D85894: [BuildLibCalls] Add more noundef to library functions.
Thu, Aug 13, 4:41 AM · Restricted Project
aqjune requested review of D85894: [BuildLibCalls] Add more noundef to library functions.
Thu, Aug 13, 4:40 AM · Restricted Project

Tue, Aug 11

aqjune committed rG63b5b92bc958: [LazyValueInfo] Let getEdgeValueLocal look into freeze instructions (authored by aqjune).
[LazyValueInfo] Let getEdgeValueLocal look into freeze instructions
Tue, Aug 11, 12:40 AM
aqjune closed D84629: [LazyValueInfo] Let getEdgeValueLocal look into freeze instructions.
Tue, Aug 11, 12:39 AM · Restricted Project
aqjune updated the summary of D84629: [LazyValueInfo] Let getEdgeValueLocal look into freeze instructions.
Tue, Aug 11, 12:39 AM · Restricted Project
aqjune added inline comments to D85593: [InstCombine] ~(~X + Y) -> X - Y.
Tue, Aug 11, 12:27 AM · Restricted Project

Mon, Aug 10

aqjune added inline comments to D85593: [InstCombine] ~(~X + Y) -> X - Y.
Mon, Aug 10, 11:13 PM · Restricted Project
aqjune added a comment to D85593: [InstCombine] ~(~X + Y) -> X - Y.

To prove correctness of a transformation of expressions with constants and undefs, following these steps is sufficient:
(1) Replace an undef with a set of all possible values. For example, i8 undef is {0, 1, 2, ..., 255}.
(2) Evaluate each operation by pairwisely picking elements from operands' sets.
(3) If the final result is in a subset relation, the transformation is correct.

Mon, Aug 10, 11:11 PM · Restricted Project
aqjune added a comment to D85684: [InstSimplify] Forbid undef folds in expandBinOp.

Why is the test not part of this?

Mon, Aug 10, 6:40 PM · Restricted Project
aqjune accepted D85684: [InstSimplify] Forbid undef folds in expandBinOp.

I think this patch is straightforward. I'll accept this; could anyone have a look as well?

Mon, Aug 10, 6:26 PM · Restricted Project
aqjune added a comment to D85684: [InstSimplify] Forbid undef folds in expandBinOp.

Does this patch need an update at llvm/test/Transforms/InstSimplify/select.ll as well? (relevant commit: https://github.com/llvm/llvm-project/commit/566a66703f020996d07191323ae8ad6f7ad827b3 )

Mon, Aug 10, 6:22 PM · Restricted Project
aqjune added inline comments to D84629: [LazyValueInfo] Let getEdgeValueLocal look into freeze instructions.
Mon, Aug 10, 6:18 PM · Restricted Project
aqjune updated the diff for D84629: [LazyValueInfo] Let getEdgeValueLocal look into freeze instructions.

Remove redundant check

Mon, Aug 10, 6:16 PM · Restricted Project
aqjune added a comment to D85647: [InstCombine] eliminate a pointer cast around insertelement.

I agree this transformation is safe modulo the pointer size check. It should be fine to fold p2i(i2p i) -> i when the size of pointer is large enough.

Mon, Aug 10, 6:05 PM · Restricted Project
aqjune added a comment to D85345: [BuildLibCalls] Add noundef to standard I/O functions.

What about undef or poison is given to malloc? If it should raise UB, the size argument and returned pointer should be noundef.

It is unclear to me if we want to forbid undef to be passed to malloc. It makes practical sense but not from a semantic perspective.
However, even if you pass undef you should not get undef back. So the return should never be undef for sure. If it doesn, how could you ever deal with it, given that a branch on the result would be UB :D

Mon, Aug 10, 12:49 AM · Restricted Project, Restricted Project
aqjune added a comment to D85393: [WIP] [IR] Adding noprogress as a LLVM IR attribute.

Let's first create the Lang Ref patch before we dive into details here. Generally, we will need verifier checks, yes. FWIW, noprogress is unrelated to willreturn and noreturn though.

Mon, Aug 10, 12:45 AM · Restricted Project

Sun, Aug 9

aqjune added a comment to D84629: [LazyValueInfo] Let getEdgeValueLocal look into freeze instructions.

Since D84940 can introduce freezes, this patch can be effective now IMO.

Sun, Aug 9, 10:58 PM · Restricted Project
aqjune added a comment to D84242: [ValueTracking] Add UndefOrPoison/Poison-only version of relevant functions.

ping

Sun, Aug 9, 10:21 PM · Restricted Project
aqjune added a comment to D85393: [WIP] [IR] Adding noprogress as a LLVM IR attribute.

Do we need a well-formedness check at Verifier.cpp stating that noprogress and willreturn cannot exist together?

Sun, Aug 9, 10:16 PM · Restricted Project
aqjune added a comment to D85345: [BuildLibCalls] Add noundef to standard I/O functions.

What about undef or poison is given to malloc? If it should raise UB, the size argument and returned pointer should be noundef.

Sun, Aug 9, 8:59 PM · Restricted Project, Restricted Project
aqjune added a comment to D85345: [BuildLibCalls] Add noundef to standard I/O functions.

If things go well, I'll add noundef to other library functions such as utime, setenv, unsetenv, .... as well.
For malloc/calloc/realloc/free, I'll think more about them. I guess it is safe to tag noundef to free's operand, but for malloc I'm not 100% sure...

Sun, Aug 9, 7:07 PM · Restricted Project, Restricted Project
aqjune committed rGef018cb65c98: [BuildLibCalls] Add noundef to standard I/O functions (authored by aqjune).
[BuildLibCalls] Add noundef to standard I/O functions
Sun, Aug 9, 6:59 PM
aqjune closed D85345: [BuildLibCalls] Add noundef to standard I/O functions.
Sun, Aug 9, 6:58 PM · Restricted Project, Restricted Project
aqjune updated the diff for D85345: [BuildLibCalls] Add noundef to standard I/O functions.

Update PR3589-freestanding-libcalls.c

Sun, Aug 9, 6:58 PM · Restricted Project, Restricted Project
aqjune added a comment to D84941: [JumpThreading] Let ProcessImpliedCondition look into freeze instructions.

ping

Sun, Aug 9, 8:11 AM · Restricted Project
aqjune added a comment to D65718: [LangRef] Document forward-progress requirement.

the compiled program must only exhibit behaviors that were possible behaviors of the source program

That's good. Now define what the possible behaviours of the source program are. We can skip this question all the way to what causes the infinite loop to be executed before the statement following it, and not after. There needs to be some form of happens-before rule.

...
The order of execution of:

while n != 0 {n += 2;}
return 0

is harder to define, because return 0 does not depend on the computation of n. What happens-before rule would you add that should require this specific loop to be executed before this specific return of a constant, and yet not forbid other execution reorderings that the optimizer should be allowed to do?

Sun, Aug 9, 7:18 AM · Restricted Project

Sat, Aug 8

aqjune added inline comments to D85345: [BuildLibCalls] Add noundef to standard I/O functions.
Sat, Aug 8, 11:44 PM · Restricted Project, Restricted Project

Fri, Aug 7

aqjune committed rGb6d9add71b1a: [InstCombine] Optimize select(freeze(icmp eq/ne x, y), x, y) (authored by aqjune).
[InstCombine] Optimize select(freeze(icmp eq/ne x, y), x, y)
Fri, Aug 7, 11:23 PM
aqjune closed D85533: [InstCombine] Optimize select(freeze(icmp eq/ne x, y), x, y).
Fri, Aug 7, 11:22 PM · Restricted Project
aqjune committed rG595d3b5ecc59: [InstCombine] Add tests for select(freeze(icmp x, y), x, y); NFC (authored by aqjune).
[InstCombine] Add tests for select(freeze(icmp x, y), x, y); NFC
Fri, Aug 7, 11:18 PM
aqjune added a comment to D85533: [InstCombine] Optimize select(freeze(icmp eq/ne x, y), x, y).

Is there a reason to do this in InstCombine rather than InstSimplify? We are not creating any new instructions.

Fri, Aug 7, 10:38 PM · Restricted Project
aqjune added a comment to D85533: [InstCombine] Optimize select(freeze(icmp eq/ne x, y), x, y).

Added a missing condition which is that freeze should be used by the select instruction only.

Fri, Aug 7, 10:30 AM · Restricted Project
aqjune updated the summary of D85533: [InstCombine] Optimize select(freeze(icmp eq/ne x, y), x, y).
Fri, Aug 7, 10:09 AM · Restricted Project
aqjune updated the diff for D85533: [InstCombine] Optimize select(freeze(icmp eq/ne x, y), x, y).

freeze isn't needed in an output

Fri, Aug 7, 10:09 AM · Restricted Project
aqjune added inline comments to D85533: [InstCombine] Optimize select(freeze(icmp eq/ne x, y), x, y).
Fri, Aug 7, 9:53 AM · Restricted Project
aqjune updated the diff for D84940: [JumpThreading] Conditionally freeze its condition when unfolding select.

Update msan comment

Fri, Aug 7, 9:21 AM · Restricted Project
aqjune requested review of D85534: Enable InsertFreeze flag of JumpThreading when used in LTO.
Fri, Aug 7, 9:19 AM · Restricted Project
aqjune abandoned D84250: [InstSimplify] ExpandBinOp should fold an expression only when it's safe.

I don't have a preference, but others said they would like go with D84792, so let's do that.

Fri, Aug 7, 9:16 AM · Restricted Project
aqjune added a comment to D84940: [JumpThreading] Conditionally freeze its condition when unfolding select.

With the last patch applied, the performance change is within a reasonable range, except 523.xalancbmk_r . However, on my machine, the result of 523.xalancbmk_r varies sometimes without clear reason.

Fri, Aug 7, 9:06 AM · Restricted Project
aqjune requested review of D85533: [InstCombine] Optimize select(freeze(icmp eq/ne x, y), x, y).
Fri, Aug 7, 9:01 AM · Restricted Project
aqjune added a comment to D84250: [InstSimplify] ExpandBinOp should fold an expression only when it's safe.

Should we go to D84792's direction?
If it is, I think we should mention that it is a workaround for the miscompilations including PR33165, and it can still be problematic in theory but chose to do it for performance.

Fri, Aug 7, 7:22 AM · Restricted Project
aqjune added a comment to D85522: [Loads] Globals are dereferenceable, if offset + size <= sizeof(global)..

It may be a silly question, but can lifetime.end intrinsics be applied to global variables, too?
LangRef isn't clear about this, but it makes sense to say that lifetime.end only works for alloca/mallocs IMO.

Fri, Aug 7, 7:18 AM · Restricted Project

Thu, Aug 6

aqjune committed rGc771087161f4: [InstCombine] Fold freeze(undef) into a proper constant (authored by aqjune).
[InstCombine] Fold freeze(undef) into a proper constant
Thu, Aug 6, 2:40 AM
aqjune closed D84948: [InstCombine] Fold freeze(undef) into a proper constant.
Thu, Aug 6, 2:40 AM · Restricted Project
aqjune committed rG54a1097b8373: [InstCombine] Add tests for D84948; NFC (authored by aqjune).
[InstCombine] Add tests for D84948; NFC
Thu, Aug 6, 2:30 AM
aqjune retitled D84948: [InstCombine] Fold freeze(undef) into a proper constant from [InstCombine] Fold select/and/or with freeze(undef) that is used in the op only to [InstCombine] Fold freeze(undef) into a proper constant.
Thu, Aug 6, 12:32 AM · Restricted Project

Wed, Aug 5

aqjune updated the diff for D84948: [InstCombine] Fold freeze(undef) into a proper constant.

Iterate over uses of freeze(undef) and choose the best value

Wed, Aug 5, 10:40 PM · Restricted Project
aqjune added a comment to D84948: [InstCombine] Fold freeze(undef) into a proper constant.

To expand on this, if we want to handle multi-use freeze undef in the future, there's generally two ways that can work:

  1. Individual folds for certain operators (as done here), but replacing the freeze itself with an advantageous value rather than just the operator.
  2. Walking the uses of the freeze to find the best replacement value and replace all uses. (E.g. by counting for how many uses a certain value is favorable.)

The current patch would be the starting point for 1, my suggestion would be the starting point for 2.

I should say though that I'm also ok with your current patch.

Wed, Aug 5, 6:24 PM · Restricted Project
aqjune committed rG9f717d7b941f: [JumpThreading] Allow duplicating a basic block into preds when its branch… (authored by aqjune).
[JumpThreading] Allow duplicating a basic block into preds when its branch…
Wed, Aug 5, 5:52 PM
aqjune committed rGfd86d67b8283: [JumpThreading] Add a test that duplicates insts of a basic block with cond br… (authored by aqjune).
[JumpThreading] Add a test that duplicates insts of a basic block with cond br…
Wed, Aug 5, 5:51 PM
aqjune closed D85029: [JumpThreading] Allow duplicating a basic block into preds when its branch condition is freeze(phi).
Wed, Aug 5, 5:51 PM · Restricted Project
aqjune updated the summary of D85029: [JumpThreading] Allow duplicating a basic block into preds when its branch condition is freeze(phi).
Wed, Aug 5, 5:51 PM · Restricted Project
aqjune added a comment to D85332: [SCCP] Do not replace deref'able ptr with un-deref'able one..

Note that Alive2 seems to think that the replacement is valid:
https://alive2.llvm.org/ce/z/2rorhk

Wed, Aug 5, 12:39 PM · Restricted Project
aqjune updated the summary of D85345: [BuildLibCalls] Add noundef to standard I/O functions.
Wed, Aug 5, 12:36 PM · Restricted Project, Restricted Project
aqjune updated the summary of D85345: [BuildLibCalls] Add noundef to standard I/O functions.
Wed, Aug 5, 12:36 PM · Restricted Project, Restricted Project
aqjune added a comment to D85184: [Attributor][WIP] Deduce noundef attribute.

BuildLibCalls.cpp 's inferLibFuncAttributes seems to be the right place for this?
I'll start with adding noundef to return values of library functions.

That would be great, and yest that is the right place for most of it IIRC.

Wed, Aug 5, 12:34 PM · Restricted Project
aqjune requested review of D85345: [BuildLibCalls] Add noundef to standard I/O functions.
Wed, Aug 5, 12:31 PM · Restricted Project, Restricted Project
aqjune added inline comments to D85029: [JumpThreading] Allow duplicating a basic block into preds when its branch condition is freeze(phi).
Wed, Aug 5, 12:32 AM · Restricted Project
aqjune updated the diff for D85029: [JumpThreading] Allow duplicating a basic block into preds when its branch condition is freeze(phi).

Update the comment

Wed, Aug 5, 12:31 AM · Restricted Project

Tue, Aug 4

aqjune committed rGe0d99e9aaf51: [JumpThreading] Consider freeze as a zero-cost instruction (authored by aqjune).
[JumpThreading] Consider freeze as a zero-cost instruction
Tue, Aug 4, 10:43 PM
aqjune committed rG3401f9706be1: [JumpThreading] Add a test for D85023; NFC (authored by aqjune).
[JumpThreading] Add a test for D85023; NFC
Tue, Aug 4, 10:43 PM
aqjune closed D85023: [JumpThreading] Consider freeze as a zero-cost instruction.
Tue, Aug 4, 10:43 PM · Restricted Project
aqjune added a comment to D85184: [Attributor][WIP] Deduce noundef attribute.

Note: @aqjune, and @okura We should also seed noundef in the intrinsics and especially known library functions definitions. I mean, malloc is not returning a undef or poison value (I hope), and free is not accepting one.

Tue, Aug 4, 9:55 PM · Restricted Project
aqjune committed rG998c0efee0e6: [JumpThreading] Update test freeze.ll; NFC (authored by aqjune).
[JumpThreading] Update test freeze.ll; NFC
Tue, Aug 4, 4:28 AM
aqjune committed rGe734e8286b4b: [JumpThreading] Remove cast's constraint (authored by aqjune).
[JumpThreading] Remove cast's constraint
Tue, Aug 4, 3:10 AM
aqjune committed rGe218da7ff39d: [JumpThreading] Add a test for simplification of cast of any op; NFC (authored by aqjune).
[JumpThreading] Add a test for simplification of cast of any op; NFC
Tue, Aug 4, 3:09 AM
aqjune closed D85188: [JumpThreading] Remove cast's constraint.
Tue, Aug 4, 3:09 AM · Restricted Project
aqjune requested review of D85188: [JumpThreading] Remove cast's constraint.
Tue, Aug 4, 2:28 AM · Restricted Project
aqjune added inline comments to D85029: [JumpThreading] Allow duplicating a basic block into preds when its branch condition is freeze(phi).
Tue, Aug 4, 1:57 AM · Restricted Project
aqjune updated the diff for D85029: [JumpThreading] Allow duplicating a basic block into preds when its branch condition is freeze(phi).

Rebase

Tue, Aug 4, 1:56 AM · Restricted Project
aqjune added inline comments to D85184: [Attributor][WIP] Deduce noundef attribute.
Tue, Aug 4, 1:29 AM · Restricted Project
aqjune updated the diff for D85023: [JumpThreading] Consider freeze as a zero-cost instruction.

Minor fix

Tue, Aug 4, 1:24 AM · Restricted Project
aqjune updated the diff for D85023: [JumpThreading] Consider freeze as a zero-cost instruction.

Update test

Tue, Aug 4, 1:23 AM · Restricted Project
aqjune added a comment to D85023: [JumpThreading] Consider freeze as a zero-cost instruction.

I merged the tests to a single file (https://github.com/llvm/llvm-project/commit/1ea84653378132091b5b6d31d4f6bf3ec7da7b56 ).
Two tests required targets that are different from x86_64-unknown-linux-gnu , so they are left unmerged.

Tue, Aug 4, 1:10 AM · Restricted Project
aqjune committed rG1ea846533781: [JumpThreading] Merge/rename thread-two-bbsN.ll tests; NFC (authored by aqjune).
[JumpThreading] Merge/rename thread-two-bbsN.ll tests; NFC
Tue, Aug 4, 1:08 AM
aqjune committed rG6f97103b561c: [JumpThreading] Don't limit the type of an operand (authored by aqjune).
[JumpThreading] Don't limit the type of an operand
Tue, Aug 4, 12:22 AM
aqjune closed D84949: [JumpThreading] Don't limit the type of an operand.
Tue, Aug 4, 12:22 AM · Restricted Project
aqjune updated the diff for D84949: [JumpThreading] Don't limit the type of an operand.

Update the comment

Tue, Aug 4, 12:21 AM · Restricted Project

Sat, Aug 1

aqjune added a comment to D84250: [InstSimplify] ExpandBinOp should fold an expression only when it's safe.

Assuming D84792 lands, would performing the two SimplifyBinOp queries for L and R with CanUseUndef=false solve this problem as well? I would prefer that if it works.

Sat, Aug 1, 6:00 AM · Restricted Project

Fri, Jul 31

aqjune added a comment to D84250: [InstSimplify] ExpandBinOp should fold an expression only when it's safe.

I tried to find other examples where this could happen, and I can't find anything else either.
So I think this just needs some minor changes.
However, Alive2 does not show the "f_dontfold" test example or my reduced candidate test as incorrect - is that a bug for Alive2?
https://alive2.llvm.org/ce/z/dbGLzN

f_dontfold simply shows that the patch prevents the folding; it is still correct to fold the expression. I'll leave TODO there.
I'll update this patch to reflect the comments.

Fri, Jul 31, 9:02 AM · Restricted Project
aqjune updated the diff for D84250: [InstSimplify] ExpandBinOp should fold an expression only when it's safe.

Reflect comments

Fri, Jul 31, 9:02 AM · Restricted Project
aqjune updated the summary of D84940: [JumpThreading] Conditionally freeze its condition when unfolding select.
Fri, Jul 31, 9:02 AM · Restricted Project
aqjune requested review of D85029: [JumpThreading] Allow duplicating a basic block into preds when its branch condition is freeze(phi).
Fri, Jul 31, 9:02 AM · Restricted Project
aqjune added a comment to D84250: [InstSimplify] ExpandBinOp should fold an expression only when it's safe.

I tried to find other examples where this could happen, and I can't find anything else either.
So I think this just needs some minor changes.
However, Alive2 does not show the "f_dontfold" test example or my reduced candidate test as incorrect - is that a bug for Alive2?
https://alive2.llvm.org/ce/z/dbGLzN

Fri, Jul 31, 6:33 AM · Restricted Project
aqjune updated the diff for D85023: [JumpThreading] Consider freeze as a zero-cost instruction.

Minor fix

Fri, Jul 31, 5:17 AM · Restricted Project
aqjune requested review of D85023: [JumpThreading] Consider freeze as a zero-cost instruction.
Fri, Jul 31, 5:15 AM · Restricted Project
aqjune added inline comments to D84949: [JumpThreading] Don't limit the type of an operand.
Fri, Jul 31, 4:25 AM · Restricted Project
aqjune added inline comments to D84949: [JumpThreading] Don't limit the type of an operand.
Fri, Jul 31, 1:34 AM · Restricted Project
aqjune updated the diff for D84941: [JumpThreading] Let ProcessImpliedCondition look into freeze instructions.

Fix another failing test

Fri, Jul 31, 1:23 AM · Restricted Project
aqjune added a comment to D84948: [InstCombine] Fold freeze(undef) into a proper constant.

Hi, sorry for delay.

Fri, Jul 31, 1:21 AM · Restricted Project

Thu, Jul 30

aqjune added inline comments to D84941: [JumpThreading] Let ProcessImpliedCondition look into freeze instructions.
Thu, Jul 30, 11:56 PM · Restricted Project
aqjune added a comment to D84944: [JumpThreading] Let SimplifyPartiallyRedundantLoad look into freeze.

Since this can be tested independent from D84940, it is landed early

Thu, Jul 30, 11:30 PM · Restricted Project
aqjune committed rGad48367722b2: [JumpThreading] Let SimplifyPartiallyRedundantLoad look into freeze (authored by aqjune).
[JumpThreading] Let SimplifyPartiallyRedundantLoad look into freeze
Thu, Jul 30, 11:28 PM
aqjune closed D84944: [JumpThreading] Let SimplifyPartiallyRedundantLoad look into freeze.
Thu, Jul 30, 11:28 PM · Restricted Project
aqjune updated the diff for D84944: [JumpThreading] Let SimplifyPartiallyRedundantLoad look into freeze.

Leave pre-load.ll's diff only

Thu, Jul 30, 11:27 PM · Restricted Project
aqjune committed rGf561713d7513: [JumpThreading] Add a test for D84944 ; NFC (authored by aqjune).
[JumpThreading] Add a test for D84944 ; NFC
Thu, Jul 30, 11:21 PM
aqjune updated the diff for D84940: [JumpThreading] Conditionally freeze its condition when unfolding select.

Remove the !InsertFreezeWhenUnfoldingSelect check

Thu, Jul 30, 6:06 PM · Restricted Project
aqjune added inline comments to D84940: [JumpThreading] Conditionally freeze its condition when unfolding select.
Thu, Jul 30, 6:04 PM · Restricted Project
aqjune added a comment to D84940: [JumpThreading] Conditionally freeze its condition when unfolding select.

I'm not sure about your plan for enabling this. You're going to get very little practical test coverage enabling this for "LTO". (Big projects are much more likely to be using ThinLTO.) And long-term, we don't to have a "please miscompile my code" argument for the JumpThreading pass.

Thu, Jul 30, 6:00 PM · Restricted Project