Page MenuHomePhabricator

junryoungju (Jun Ryung Ju)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 9 2017, 2:37 AM (102 w, 9 h)

Recent Activity

Oct 31 2017

junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

The problem is, if under ICmp's operand node has a ICmp can evolutable? (or any other conditional expressions).
This only can be solved by registering this chain into getSCEV.

; RUN: opt -analyze -scalar-evolution < %s | FileCheck %s --check-prefix=CHECK-ANALYSIS

define i32 @foo() {
  br label %do_start

do_start:
  %inc = phi i32 [ 0, %0 ], [ %add, %do_start ]
  %cmp = icmp slt i32 %inc, 10000
  %add_cond = add nsw i32 %inc, 2
  %sel = select i1 %cmp, i32 %add_cond, i32 %inc
  %add = add nsw i32 %sel, 1
  br i1 %cmp, label %do_start, label %do_end

; CHECK-ANALYSIS: Loop %do_start: backedge-taken count is 3334
; CHECK-ANALYSIS: Loop %do_start: max backedge-taken count is 3334
; CHECK-ANALYSIS: Loop %do_start: Predicated backedge-taken count is 3334

do_end:
  ret i32 0
}

This case is "actaully" generated from Clang executable using -O3 -S -emit-llvm option.
that mean this case can be "always" appear.

To fix this case, we need to create SCEVConditional. that I commented here.

Okay, just we need to make sure we are evoluting conditional instructions or just checking that instructions are true or false for now.

first one.
If we are "actaully" evoluting a conditional instructions. better creating SCEVConditonal, checking that is true or false when is computing backedge taken count on each AddRec step.
I think this is a correct way for handling conditional instructions.
but, this will cause SCEV jobs MUUUUCHHH SLOWER.

second one.
If we are just checking that instructions are true or false. we do not need to rewrite it, rewriting value means that we handle that value as that "actual" SCEV. (register SCEV on SCEVCallbackVH)
this mean, this is same to write this chain on to createSCEV that we did at first.
so we have to handle it as a "except" of SCEV chain. better inlining code into createAddRecFromPHI.

and just note this issue CANNOT be solved just by checking ICmp has conditional instruction.

Just try pulling out earlier diff : https://reviews.llvm.org/D38494?id=120193 , this was fixing your new case.
You shuld attempt an incremental fix once this patch goes through.

Oct 31 2017, 1:43 AM

Oct 30 2017

junryoungju added inline comments to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..
Oct 30 2017, 10:37 PM
junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

The problem is, if under ICmp's operand node has a ICmp can evolutable? (or any other conditional expressions).
This only can be solved by registering this chain into getSCEV.

Oct 30 2017, 10:31 PM
junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

:( that I commented didn't reviewed. this is why I wanted to make other reversion.

Are you saying that you had made the same comments as I did above? If so, that wasn't clear from what you wrote. You can help authors by:

  • Adding inline comments instead of a top level comment, knowing which part of the patch you're referring to helps comprehension
  • Being specific; instead of saying "this is very general" say "use cast instead of switch"
Oct 30 2017, 4:18 PM
junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

:( that I commented didn't reviewed. this is why I wanted to make other reversion.

Oct 30 2017, 1:52 AM

Oct 26 2017

junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

Okay, just we need to make sure we are evoluting conditional instructions or just checking that instructions are true or false for now.

Oct 26 2017, 7:04 PM
junryoungju abandoned D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

Few general concerns.

1/  Always submit patch with few test cases (valid for any patch).
2/  Patch https://reviews.llvm.org/D38494 is taking care of this problem. I don't see what is your point in duplicating the efforts.

You are on the review list also.

There is no derth of open problems to tackle. I hope you understand my point here.

Thanks.

Because we are solving issue as another way.

I see you have already joined the review of the other patch. If there is a problem with that approach, please clearly explain why and what the alternative approach would be on that patch as part of code review. Perhaps the author will wish you to provide a patch for this alternative, but it doesn't make sense to continue reviewing *both* patches in parallel. We should figure out which approach is best first, and then review the patch that uses that approach. As the other patch (D38494) seems to be quite active, I would suggest consolidating the discussion around approach on that patch.

Until that is resolved, I don't think it makes sense to continue iterating in review here. It just fragments the discussion.

Oct 26 2017, 6:56 PM
junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

This is why I noticed that this is reversion of @jbhateja 's patch

Oct 26 2017, 6:11 PM
junryoungju added inline comments to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..
Oct 26 2017, 6:10 PM

Oct 25 2017

junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

For this time, I am actually working on SwitchInst.
before I work on it, I want to check that this patch has no issue

Oct 25 2017, 6:09 PM
junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..
Oct 25 2017, 6:08 PM

Oct 24 2017

junryoungju abandoned D37672: newest version of rc.exe (resource compiler) emits error with /nologo option..
Oct 24 2017, 11:30 PM
junryoungju updated the diff for D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

Fixed wrong testcase uploaded.

Oct 24 2017, 6:56 PM
junryoungju updated the diff for D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

Fixed testcase checks.

Oct 24 2017, 6:32 PM
junryoungju updated the diff for D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

Now we evolute SelectInst directly.
Also added some testcases.

Oct 24 2017, 6:27 PM
junryoungju added a comment to D37672: newest version of rc.exe (resource compiler) emits error with /nologo option..

ping

Oct 24 2017, 12:39 AM
junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

@reviewers, kindly let me know if any other comments.

Oct 24 2017, 12:34 AM
junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

Few general concerns.

1/  Always submit patch with few test cases (valid for any patch).
2/  Patch https://reviews.llvm.org/D38494 is taking care of this problem. I don't see what is your point in duplicating the efforts.

You are on the review list also.

There is no derth of open problems to tackle. I hope you understand my point here.

Thanks.

Oct 24 2017, 12:27 AM

Oct 23 2017

junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

Did you checked that this fixes emits right assemblies on clang?
sometimes it doesn't optimizes even it emits correct loop-count on opt

Oct 23 2017, 4:23 AM

Oct 22 2017

junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

Now this checks that ICmp is false or true cond.

Oct 22 2017, 11:06 PM
junryoungju updated the diff for D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..
Oct 22 2017, 11:05 PM

Oct 17 2017

junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..
Oct 17 2017, 11:37 PM
junryoungju retitled D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain. from [ScalarEvolution] Handling for ICmp occuring in the evolution chain. to [ScalarEvolution] Handling Conditional Instruction in SCEV chain..
Oct 17 2017, 8:33 PM
junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..
  • Please rebase this patch with trunk.
  • Add a test case which asserts on scalar evoluition dumps.

    It shall expedite the review process.
Oct 17 2017, 8:30 PM
junryoungju updated subscribers of D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..
Oct 17 2017, 6:52 PM
junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

@jbhateja

Oct 17 2017, 6:49 PM
junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

This will handle that https://reviews.llvm.org/D38494 cannot handle.

Oct 17 2017, 6:49 PM
junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

I think you should apply patch and create new diff with full-context :<

Oct 17 2017, 3:56 PM
junryoungju accepted D38494: [SCEV] Handling for ICmp occuring in the evolution chain..
Oct 17 2017, 9:29 AM
junryoungju accepted D38494: [SCEV] Handling for ICmp occuring in the evolution chain..
Oct 17 2017, 6:19 AM
junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

:>

Oct 17 2017, 6:18 AM
junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

Go to diff history, and download diff. and re upload it.

Oct 17 2017, 6:02 AM
junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

I don't understand why is there are CG commits here....

Oct 17 2017, 5:51 AM
junryoungju updated the diff for D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

Rollback patch.

Oct 17 2017, 4:15 AM
junryoungju accepted D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

NVM All looks fine.

Oct 17 2017, 4:12 AM
junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

Hi Jun Ryoung Ju,

There is a open revision under review https://reviews.llvm.org/D38494 on this.

It seems fix here are inspired from that revision which is an overlap, revision is already open and its comments have been addressed.

Thanks

Oct 17 2017, 3:49 AM
junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..
Oct 17 2017, 3:46 AM

Oct 16 2017

junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

It should be like this

while.cond:                                       ; preds = %sw.epilog, %entry
  %step.0 = phi i32 [ 0, %entry ], [ %step.1, %sw.epilog ]
  switch i32 %step.0, label %sw.epilog [
    i32 0, label %sw.bb
    i32 1, label %sw.bb1
    i32 2, label %sw.bb2
    i32 3, label %sw.bb3
  ]
Oct 16 2017, 10:11 PM
junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

But still it doesn't handle if its not a ICmpInst or SelectInst.
We should also handle SwitchInst.

Oct 16 2017, 9:53 PM
junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

Fixed that checking SelectInst will not return nullptr if loop isn't same.

Oct 16 2017, 9:52 PM
junryoungju updated the diff for D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..
Oct 16 2017, 9:47 PM
junryoungju updated the diff for D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

Now emits correct backedge-taken count.

Oct 16 2017, 9:34 PM
junryoungju added inline comments to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..
Oct 16 2017, 9:10 PM
junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

@sanjoy its done :> review please.

Oct 16 2017, 8:00 PM
junryoungju updated the diff for D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

Cleanup, Refactored code.

Oct 16 2017, 7:59 PM
junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

NOTE : DO NOT MERGE OR ACCEPT THIS. FIXING DIFF.

Oct 16 2017, 12:55 AM
junryoungju updated the diff for D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

Now do not handle that we cannot find exit condition. also, refactored.

Oct 16 2017, 12:53 AM

Oct 15 2017

junryoungju retitled D37672: newest version of rc.exe (resource compiler) emits error with /nologo option. from newest version of rc.exe (resource compiler) doesn't supports /nologo to newest version of rc.exe (resource compiler) emits error with /nologo option..
Oct 15 2017, 4:26 PM
junryoungju added inline comments to D37672: newest version of rc.exe (resource compiler) emits error with /nologo option..
Oct 15 2017, 4:25 PM

Oct 14 2017

junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

this will handle checking AddExpr increasing ICmp's target operand, also now handles if AddExpr increasing an Constant, we are going to use it.

Oct 14 2017, 7:41 PM
junryoungju updated the diff for D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..
Oct 14 2017, 7:40 PM
junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

I don't understand what is happening right now D:

Oct 14 2017, 6:08 PM

Oct 13 2017

junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

current trunk keep build fails D: I ill upload when its available.

Oct 13 2017, 6:07 PM

Oct 12 2017

junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

so that I have to change this code to emit SCEVAddRecExpr on createAddRecFromPHI right?
not evoluting SCEVUnknown.

Oct 12 2017, 11:34 PM
junryoungju updated the diff for D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..
Oct 12 2017, 6:46 PM
junryoungju updated the diff for D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

Now check that AddExpr on ICmp's PHI Node modifies ICmp's comparer operand.

Oct 12 2017, 6:45 PM
junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

I went through your example, and I think I know what you need to do -- while you cannot assume that a condition feeding into the loop latch is true generally, you can assume it is true *in the context* of a add recurrence increment. That is, in createAddRecFromPHI, under // Create an add with everything but the specified operand., if an operand to the addition is a loop variant value that is the loop latch condition then you can assume said loop variant value to be the constant 1.

Yes, but. if I understood correctly, isn't it assuming from context seems does not match with SCEV's concept?
It will just analyze all operand, not evoluting child operand nodes.

so I was tried to evolute "Latch is taken, but cannot be evoluted as SCEVAddRecExpr" generally that emitted as "SCEVUnknown" operand with ICmp.
this mean, this will not evolute general ICmp instruction that has correct SCEV node that resolved as SCEVAddRecExpr operand.

Oct 12 2017, 6:05 PM

Oct 11 2017

junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

I went through your example, and I think I know what you need to do -- while you cannot assume that a condition feeding into the loop latch is true generally, you can assume it is true *in the context* of a add recurrence increment. That is, in createAddRecFromPHI, under // Create an add with everything but the specified operand., if an operand to the addition is a loop variant value that is the loop latch condition then you can assume said loop variant value to be the constant 1.

Oct 11 2017, 6:23 PM
junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

I don't understand what you mean D:
does it meaning do I need to upload raw source code?

Oct 11 2017, 4:52 PM
junryoungju added a comment to D37672: newest version of rc.exe (resource compiler) emits error with /nologo option..

Review this please. https://bugs.llvm.org/show_bug.cgi?id=33493

Oct 11 2017, 1:38 AM
junryoungju updated subscribers of D37672: newest version of rc.exe (resource compiler) emits error with /nologo option..
Oct 11 2017, 1:38 AM
junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

And here is emitted assemblies with following code. ( clang -O3 -S )

Oct 11 2017, 1:33 AM
junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

Reversion of https://reviews.llvm.org/D38494

Oct 11 2017, 1:24 AM
junryoungju updated the summary of D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..
Oct 11 2017, 1:22 AM
junryoungju added a comment to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..

Lol I just did reversion old patch.
just ignore old comments or patch.

Oct 11 2017, 1:20 AM
junryoungju updated the diff for D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..
Oct 11 2017, 1:18 AM
junryoungju added a comment to D37672: newest version of rc.exe (resource compiler) emits error with /nologo option..

ping

Oct 11 2017, 1:09 AM

Oct 10 2017

junryoungju added a reviewer for D37672: newest version of rc.exe (resource compiler) emits error with /nologo option.: rnk.
Oct 10 2017, 9:10 PM

Oct 4 2017

junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

Yes, that is what I meant,
this will not evoluate all of loops at all.

Oct 4 2017, 3:12 AM

Oct 3 2017

junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..
int values[11];
int i = 0;
do {
Oct 3 2017, 7:54 PM
junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

(before I explaining it, please understand that I am not really good at english)
I meant It will not be executed before it has something affects on any other variables. (check out https://bugs.llvm.org/show_bug.cgi?id=34538)
because that issue isn't target on that you meant. (issue of LoopDeletion)
in LoopDeletion, getBackedgeTakenInfo will return SCEVUnkown correctly if it affects on any other outside variable (because it can be infinite loop getBackedgeTakenInfo cannot analysis maximum backedge count)

Oct 3 2017, 7:49 PM
junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

ah I misunderstood.

Oct 3 2017, 7:39 PM
junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

no, If there has no latch will not execute evoluateForICmp in LoopDeletion(if I think correct)
getBackedgeTakenInfo will return SCEVUnkown correctly if there is no latch.

Oct 3 2017, 7:36 PM
junryoungju added a comment to D38494: [SCEV] Handling for ICmp occuring in the evolution chain..
Oct 3 2017, 7:17 PM
junryoungju accepted D38494: [SCEV] Handling for ICmp occuring in the evolution chain..

Looks fine.

Oct 3 2017, 3:39 AM

Sep 11 2017

junryoungju accepted D37672: newest version of rc.exe (resource compiler) emits error with /nologo option..
Sep 11 2017, 12:47 AM
junryoungju added a comment to D37672: newest version of rc.exe (resource compiler) emits error with /nologo option..
Sep 11 2017, 12:46 AM
junryoungju planned changes to D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..
Sep 11 2017, 12:43 AM

Sep 10 2017

junryoungju created D37672: newest version of rc.exe (resource compiler) emits error with /nologo option..
Sep 10 2017, 7:56 PM

Sep 9 2017

junryoungju created D37660: [ScalarEvolution] Handling Conditional Instruction in SCEV chain..
Sep 9 2017, 2:54 AM