Page MenuHomePhabricator

bin.narwal (bin)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 20 2019, 6:16 AM (12 w, 6 d)

Recent Activity

Yesterday

bin.narwal created D70361: [Polly][ScopInfo]Fix wrong map in updating AccessRelation of multi-element access.
Sun, Nov 17, 3:48 AM · Restricted Project, Restricted Project

Tue, Nov 12

bin.narwal added a comment to D70076: [Polly][CodeGen]Fix getArrayAccessFor crashes as in bug 32534 with -polly-vectorizer=polly.

Thanks for looking into the bug. The suggestion change is obvious (IMHO).

However, the test case seems to be the unmodified reduced test case from bugzilla. Generally, committed test cases should be

  1. Minimal (No unnecessary comments, metadata, etc.)
  2. Have at least one positive CHECK line.

    But the change is obvious enough, I'd commit it without test-case.
Tue, Nov 12, 4:55 AM · Restricted Project, Restricted Project

Mon, Nov 11

bin.narwal created D70076: [Polly][CodeGen]Fix getArrayAccessFor crashes as in bug 32534 with -polly-vectorizer=polly.
Mon, Nov 11, 5:38 AM · Restricted Project, Restricted Project

Oct 18 2019

bin.narwal added a comment to D68941: [ScopBuilder]Fix bug 38358 by preserve correct order of ScopStmts.

Thanks for that patch!

Unfortunately I forgot the "Patch by" line. Please excuse the neglectfulness.

Oct 18 2019, 7:10 PM · Restricted Project, Restricted Project

Oct 15 2019

bin.narwal added a comment to D68941: [ScopBuilder]Fix bug 38358 by preserve correct order of ScopStmts.

I remember I cared about the epilogue being the last, which is the reason for the reversed list. It ensures that the terminator is always visited first, hence added first into the list, hence being the last statement. (I am a bit surprised that no test fails). An example could be:

%b = load %B  // merged with terminator by joinOperandTree
store 42 // independent statement
br %b // terminator, epilogue

Seeing this example, it is clear that load %B (and therefore the epilogue) cannot be put after store 42, as it might store to %A. That is, the epilogue is not necessarily always the last instruction. It should not matter since the PHI-writes have their own memory locations that do not alias with other memory.
[^^ just my thoughts about whether everything is sound; I could not find an issue]

That's because the code doesn't model branch instruction (which is terminator) at all right now. If in case terminator instruction must be handled, we would need to handle it as an "ordered" instruction, just like memory access ones.

Oct 15 2019, 6:09 AM · Restricted Project, Restricted Project
bin.narwal updated the diff for D68941: [ScopBuilder]Fix bug 38358 by preserve correct order of ScopStmts.

Fixed as you suggested. Yes please help commit this on behalf of me.

Oct 15 2019, 6:05 AM · Restricted Project, Restricted Project

Oct 14 2019

bin.narwal created D68941: [ScopBuilder]Fix bug 38358 by preserve correct order of ScopStmts.
Oct 14 2019, 5:19 AM · Restricted Project, Restricted Project

Sep 12 2019

bin.narwal updated the diff for D67007: [ScopBuilder]Skip getting leader when merging statements to close holes.

Sorry for delaying. Here is the updated patch as agreed. Also, please help me commit it.

Sep 12 2019, 6:26 AM · Restricted Project, Restricted Project

Sep 5 2019

bin.narwal added a comment to D67007: [ScopBuilder]Skip getting leader when merging statements to close holes.

Ahh, I think I understand the argument. Thank you for your patience. But I strongly suggest to update the description. Suggestion:

// We are backtracking from the last element until we see Inst's leader in SeenLeaders and merge all into one set. Although which instructions are leaders changes during the execution of this loop, it is irrelevant as we are just searching for the element that we already confirmed is in the list.
 for (Instruction *Prev : reverse(SeenLeaders)) {

If SetVector.insert would have returned an iterator to the found element, we could also have forward iterated from that element to the end.

Maybe also add a comment to bool Inserted = SeenLeaders.insert(Leader) such as:

// Since previous iterations might have merged sets, some items in SeenLeaders are not leaders anymore. However, former leaders represent the same set as the actually leader also in SeenLeaders and by construction are all trailing the list.

Since apparently its even hard for myself (who wrote this algorithm) to re-understand what's going on.

Sep 5 2019, 7:06 AM · Restricted Project, Restricted Project

Sep 4 2019

bin.narwal added a comment to D67007: [ScopBuilder]Skip getting leader when merging statements to close holes.

Thank you for the find. Indeed the name of PrevLeader suggests it to be the leader of Prev. It's leader should be identical to leader to the last element of SeenLeaders, but only before the first unionSet operation. Then the tail ("B") has been merged into Inst's leader "A", making the condition a tautology. That's definitely wrong!

However, I don't think we can save on the getLeaderValue call. Each of the instruction's leader may still be in SeenLeader, but the order in which it appears matter (it's a SetVector), and Inst's leader may change as well. The most explicit version I can think of is:

Yes, the order is not guaranteed, and Inst's leader could change during merging, but IIUC, these do not matter.
For the check condition:

if (Prev != Leader)

"Leader" is computed when we try to insert "Inst" into SeenLeaders. Whether leader of the "Leader" changes doesn't have impact on the loop, as long as the first "Prev" in the merging group is actually "Leader". The cons is we may do unnecessary call to unionSets because we only break at the first Prev Inst of merging group.
So looks like we can either compare real leaders of the two Insts, or just compare the two Insts it-selves.

Sep 4 2019, 7:00 AM · Restricted Project, Restricted Project

Aug 30 2019

bin.narwal created D67007: [ScopBuilder]Skip getting leader when merging statements to close holes.
Aug 30 2019, 8:18 AM · Restricted Project, Restricted Project

Aug 28 2019

bin.narwal added a comment to D66698: [ScopBuilder]Remove redundant while loop in ScopBuilder::buildDomains.

Hmm, but the while loop is redundant, right?

In my understanding of the word, "redundant" means unnecessarily doing something that has already been done. I'd call the loop "dead".

Aug 28 2019, 5:56 AM · Restricted Project, Restricted Project
bin.narwal updated the summary of D66698: [ScopBuilder]Remove redundant while loop in ScopBuilder::buildDomains.
Aug 28 2019, 5:56 AM · Restricted Project, Restricted Project

Aug 27 2019

bin.narwal added a comment to D66698: [ScopBuilder]Remove redundant while loop in ScopBuilder::buildDomains.

LGMT. L is just unused after that (not "redundant"). Can you correct the summary?

Hmm, but the while loop is redundant, right? I modified the summary mentioning that L/LD are later unused.

I can commit this for you if you would like me to.

Yes, please help commit this for me. Thanks very much.

Aug 27 2019, 4:39 AM · Restricted Project, Restricted Project
bin.narwal updated the summary of D66698: [ScopBuilder]Remove redundant while loop in ScopBuilder::buildDomains.
Aug 27 2019, 4:35 AM · Restricted Project, Restricted Project
bin.narwal updated the diff for D66741: [polly][NFC] Compute WAR dependence info using ISL kills.

Hi Michael, thanks for reviewing. I updated the diff according to your comments.
And yes, please help commit this for me.

Aug 27 2019, 4:35 AM · Restricted Project, Restricted Project

Aug 26 2019

bin.narwal created D66741: [polly][NFC] Compute WAR dependence info using ISL kills.
Aug 26 2019, 7:09 AM · Restricted Project, Restricted Project

Aug 23 2019

bin.narwal created D66698: [ScopBuilder]Remove redundant while loop in ScopBuilder::buildDomains.
Aug 23 2019, 7:45 PM · Restricted Project, Restricted Project

Aug 22 2019

bin.narwal updated the diff for D66477: Simplify main statement flag in ScopBuilder::buildEqivClassBlockStmts.

Hi Meinersbur,
Thanks very much for reviewing. Here is the updated patch according to your comments.

Aug 22 2019, 10:02 PM · Restricted Project, Restricted Project

Aug 21 2019

bin.narwal updated subscribers of D66477: Simplify main statement flag in ScopBuilder::buildEqivClassBlockStmts.
Aug 21 2019, 9:21 PM · Restricted Project, Restricted Project

Aug 20 2019

bin.narwal created D66477: Simplify main statement flag in ScopBuilder::buildEqivClassBlockStmts.
Aug 20 2019, 7:57 AM · Restricted Project, Restricted Project