Page MenuHomePhabricator

ayonam (Ayonam Ray)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 12 2018, 12:09 AM (53 w, 1 d)

Recent Activity

Mar 6 2019

ayonam committed rG2a0f2c5ef333: [CodeGen] Omit range checks from jump tables when lowering switches with… (authored by ayonam).
[CodeGen] Omit range checks from jump tables when lowering switches with…
Mar 6 2019, 2:03 AM
ayonam committed rL355490: [CodeGen] Omit range checks from jump tables when lowering switches with….
[CodeGen] Omit range checks from jump tables when lowering switches with…
Mar 6 2019, 2:03 AM

Mar 5 2019

ayonam committed rGaf92b7a3b899: Reversing the commit of revision 355483 since it is giving a regression on a… (authored by ayonam).
Reversing the commit of revision 355483 since it is giving a regression on a…
Mar 5 2019, 11:52 PM
ayonam committed rL355487: Reversing the commit of revision 355483 since it is giving a regression on a….
Reversing the commit of revision 355483 since it is giving a regression on a…
Mar 5 2019, 11:51 PM
ayonam committed rG6025fa8e3007: [CodeGen] Omit range checks from jump tables when lowering switches with… (authored by ayonam).
[CodeGen] Omit range checks from jump tables when lowering switches with…
Mar 5 2019, 11:27 PM
ayonam committed rL355483: [CodeGen] Omit range checks from jump tables when lowering switches with….
[CodeGen] Omit range checks from jump tables when lowering switches with…
Mar 5 2019, 11:27 PM
ayonam closed D52002: Omit range checks from jump tables when lowering switches with unreachable default.
Mar 5 2019, 11:27 PM · Restricted Project

Jan 29 2019

ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

Thanks! In general I suggest watching the bots carefully, and aggressively revert to green if something goes wrong.

Jan 29 2019, 7:12 AM · Restricted Project
ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

I will revert the checkin, fix the tests and recheckin.

Also please reference the review in the commit message when committing next time, by including this line: "Differential revision: https://reviews.llvm.org/D52002". That way the code review gets an update about the commit.

Jan 29 2019, 7:06 AM · Restricted Project
ayonam committed rL352504: Reversing the checkin for version 352484 as tests are failing..
Reversing the checkin for version 352484 as tests are failing.
Jan 29 2019, 7:01 AM
ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.
Jan 29 2019, 6:56 AM · Restricted Project
ayonam committed rL352484: [CodeGen] Omit range checks from jump tables when lowering switches with….
[CodeGen] Omit range checks from jump tables when lowering switches with…
Jan 29 2019, 4:02 AM

Jan 16 2019

ayonam updated the diff for D52002: Omit range checks from jump tables when lowering switches with unreachable default.

ayonam updated this revision to Diff 181000.Thu, Jan 10, 12:55
Comment Actions

Jan 16 2019, 6:51 AM · Restricted Project

Jan 14 2019

ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

Sorry for the long delay. I'm back from vacation now, and I'll commit this tomorrow morning unless anyone else objects.

Jan 14 2019, 10:01 PM · Restricted Project
ayonam updated the diff for D52002: Omit range checks from jump tables when lowering switches with unreachable default.

Updated the comment in the test case to reflect the changes made.

Jan 14 2019, 9:56 PM · Restricted Project

Jan 13 2019

ayonam abandoned D55742: Test case for patches D52002 and D52707.

This test case has been subsumed in D52707

Jan 13 2019, 9:15 PM
ayonam abandoned D55901: Test case for patch D52002.

This test case has been subsumed in D52002

Jan 13 2019, 9:13 PM

Jan 11 2019

ayonam added a comment to D52707: Switch optimization in IR for known maximum switch values.

Sorry to bother you again. Will it be possible to review this change and accept it please?

Jan 11 2019, 8:42 PM · Restricted Project
ayonam requested review of D52002: Omit range checks from jump tables when lowering switches with unreachable default.

This needs another review and acceptance since a small change was made to the original implementation and two tests had to be changed.

Jan 11 2019, 8:33 PM · Restricted Project

Jan 9 2019

ayonam updated the diff for D52002: Omit range checks from jump tables when lowering switches with unreachable default.

The patch failed another test because the branch to the unreachable default was omitted. The test cases have been modified. The affected tests are test/CodeGen/X86/pr38743.ll and test/CodeGen/X86/switch-jump-table.ll

Jan 9 2019, 11:25 PM · Restricted Project
ayonam added a comment to D56242: Elevate instructions across if-else blocks for better constant propagation.

Yes, I too am of the same opinion. However, the other two passes that hoist code are ConstantHoisting and GVNHoisting. While the former hoists constants the latter hoists expressions from branches to the common dominator. What the Elevate pass does is to hoist expressions from a join block to its predecessors if the expressions can be constant evaluated. (In effect, it does the reverse of what the Sink pass does - to sink expressions into the join block from its predecessors).

All that is a very good point, and i think it even reinforces my previous point.
I think it would be best to be consistent, and call this InstructionHoisting.

Jan 9 2019, 12:30 PM
ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

The fix in the code is at line 2204 of lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

Jan 9 2019, 12:24 PM · Restricted Project
ayonam updated the diff for D52002: Omit range checks from jump tables when lowering switches with unreachable default.

There were two tests that were failing when I tried to commit. This version fixes one of the tests and for the other test a fix in the original patch is provided.

Jan 9 2019, 12:22 PM · Restricted Project
ayonam added a comment to D52707: Switch optimization in IR for known maximum switch values.

Could you please review this patch after the changes I made? If things are fine, then can this be accepted for commit?

Jan 9 2019, 11:25 AM · Restricted Project

Jan 6 2019

ayonam added a comment to D56242: Elevate instructions across if-else blocks for better constant propagation.

Sorry, I somehow got the impression that you were also looking after the regression test framework and hence thought that you should review the test case part of it.

It is usually best to add the people who last worked on the newly changed code
(git diff --name-only --cached | xargs -n 1 git blame --porcelain | grep "^author " | sort | uniq -c | sort -nr | head -10),
(maybe change --cached to upstream/master..HEAD) and the code owners. (i'm neither here)

Jan 6 2019, 10:46 AM
ayonam updated the diff for D56242: Elevate instructions across if-else blocks for better constant propagation.

Ran clang-format on the changes and updated the diff

Jan 6 2019, 9:03 AM
ayonam added a comment to D56242: Elevate instructions across if-else blocks for better constant propagation.

Thanks for the patch! I plan to have a closer look in the next few days.

Jan 6 2019, 6:30 AM
ayonam updated subscribers of D56242: Elevate instructions across if-else blocks for better constant propagation.

Hm, why i'm the reviewer here?

Jan 6 2019, 6:25 AM

Jan 3 2019

ayonam added a comment to D52707: Switch optimization in IR for known maximum switch values.

A gentle reminder to the reviewers for review comments on this please. If there are no concerns, then can we move this to the accepted state please?

Jan 3 2019, 9:34 AM · Restricted Project

Jan 2 2019

ayonam created D56242: Elevate instructions across if-else blocks for better constant propagation.
Jan 2 2019, 8:20 PM

Dec 31 2018

ayonam committed rL350187: Reversing the commit in revision 350186. Revision causes regression in 4.
Reversing the commit in revision 350186. Revision causes regression in 4
Dec 31 2018, 11:32 PM
ayonam committed rL350186: Omit range checks from jump tables when lowering switches with unreachable.
Omit range checks from jump tables when lowering switches with unreachable
Dec 31 2018, 10:42 PM

Dec 21 2018

ayonam added a comment to D52707: Switch optimization in IR for known maximum switch values.

A gentle reminder to the reviewers for review comments on this please.

Dec 21 2018, 6:49 PM · Restricted Project
ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

Looks good to me. Do you have commit rights, or do you need someone to commit it for you?

(The change description is still a little out of date, but that can be fixed when committing, if not before.)

Dec 21 2018, 6:41 AM · Restricted Project
ayonam updated the summary of D52002: Omit range checks from jump tables when lowering switches with unreachable default.
Dec 21 2018, 6:27 AM · Restricted Project
ayonam updated the diff for D52002: Omit range checks from jump tables when lowering switches with unreachable default.

Resubmitting the patch with the corrections suggested earlier.

Dec 21 2018, 3:34 AM · Restricted Project

Dec 20 2018

ayonam added a comment to D55901: Test case for patch D52002.

llvm-commits is not CC'ed on this revision; llvm-commits is the official record for all reviews, so any patch posted without CC'ing llvm-commits essentially doesn't exist. It looks like you're going to abandon this anyway, but please be more careful in the future.

Dec 20 2018, 1:53 PM
ayonam added a comment to D55901: Test case for patch D52002.

Looks promising. Please make the suggested changes, and please move this patch into D52002. The functionality change and the test should be in the same patch.

Dec 20 2018, 1:49 PM
ayonam updated the diff for D52707: Switch optimization in IR for known maximum switch values.

Moved the test case from D55742 to this patch. Simplified the test case.

Dec 20 2018, 1:41 PM · Restricted Project
ayonam added inline comments to D52002: Omit range checks from jump tables when lowering switches with unreachable default.
Dec 20 2018, 1:34 PM · Restricted Project
ayonam updated the diff for D52002: Omit range checks from jump tables when lowering switches with unreachable default.

Updated the patch with test case. Incorporated comments by other reviewers. Simplified the test case.

Dec 20 2018, 1:32 PM · Restricted Project
ayonam retitled D52002: Omit range checks from jump tables when lowering switches with unreachable default from Switch optimization for known maximum switch values to Omit range checks from jump tables when lowering switches with unreachable default.
Dec 20 2018, 3:46 AM · Restricted Project

Dec 19 2018

ayonam retitled D55901: Test case for patch D52002 from Test case for patches D52002 to Test case for patch D52002.
Dec 19 2018, 1:40 PM
ayonam created D55901: Test case for patch D52002.
Dec 19 2018, 1:40 PM
ayonam updated the diff for D52002: Omit range checks from jump tables when lowering switches with unreachable default.

Reversed the recording of the switch widening in the IR. Now we solely rely on the default being made unreachable.

Dec 19 2018, 1:35 PM · Restricted Project
ayonam updated the diff for D52707: Switch optimization in IR for known maximum switch values.

Reversed the recording of the switch widening in the IR. Now we rely solely on the default being made unreachable.

Dec 19 2018, 1:33 PM · Restricted Project
ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

Sounds good.

And please include a test.

The one in D55742 doesn't really show the change we're discussing here, i.e. how unreachable defaults affect switch lowering.

Dec 19 2018, 4:38 AM · Restricted Project
ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

No, unreachable means unreachable and the compiler should treat it as such. There's nothing that says the program needs to exit, either with a segfault or anything else, when hitting __builtin_unreachable().

Dec 19 2018, 4:19 AM · Restricted Project

Dec 18 2018

ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

If you can provide more details about what didn't work, maybe I can help investigate. (Though I'm about to go on holiday soon, so probably not until January.)

There are three files that when compiled with this patch, generate wrong code, viz., AArch64LoadStoreOptimizer.cpp, AArch64InstrInfo.cpp and AArch64ConditionalCompares.cpp. Out of these we tried to isolate the problem with the last one. I figured out that if the functions SSACCmpConv::findConvertibleCompare() and SSACCmpConv::convert() are compiled without this patch, the code works fine. So the problem surfaces with these two routines only. There are a few switch cases in those two routines but I couldn't see anything exceptional with those except for a call to builtin_unreachable() in the default case for two of the switches and a [[clang::fallthrough]] in another. In all these three cases, I was unable to figure out how they could possibly break our assumptions. Does the builtin_unreachable() have any special semantic that we are not handling?

Does the error show with the regular lit tests, or do you have some internal test that fails?

My first guess would be that one of the "unreachable" defaults aren't actually unreachable for some input. But then they should trap in an asserts-enabled build..

Dec 18 2018, 7:49 PM · Restricted Project
ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

Phabricator has an "upload file" function... or you can just send an email with an attachment to llvm-commits.

Dec 18 2018, 7:34 PM · Restricted Project
ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

@hans
Is there a way to attach a pre-processed file to this review without affecting the files that are being reviewed?

Dec 18 2018, 7:21 AM · Restricted Project
ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

@hans

The earlier code had a problem in that if we converted every switch that had an unreachable default, to omit the check for the default values beyond the maximum switch case value given in the code, then there were some corner cases where the code generated was semantically incorrect and resulted in runtime failures. These failures are happening while compiling LLVM code itself.

We need to figure out what those failures were. If the unreachable default was indeed reachable, that's a big problem.

So I have added a boolean in the SwitchInst class to mark switches that have been widened, so that we omit the branch only for those switches.

LLVM doesn't put this kind of optimization metadata in the IR. The right thing to do is to figure out why removing the branch for unreachable default didn't work, and fix it.

If you can provide more details about what didn't work, maybe I can help investigate. (Though I'm about to go on holiday soon, so probably not until January.)

Dec 18 2018, 6:11 AM · Restricted Project

Dec 16 2018

ayonam added a comment to D52707: Switch optimization in IR for known maximum switch values.

...

Please review this and the other two patches 52002 and another one with the test case for this patch.

Both the D52002 and this one, still do not have any tests..

Dec 16 2018, 8:29 AM · Restricted Project
ayonam created D55742: Test case for patches D52002 and D52707.
Dec 16 2018, 8:26 AM
ayonam updated the diff for D52002: Omit range checks from jump tables when lowering switches with unreachable default.

The earlier code had a problem in that if we converted every switch that had an unreachable default, to omit the check for the default values beyond the maximum switch case value given in the code, then there were some corner cases where the code generated was semantically incorrect and resulted in runtime failures. These failures are happening while compiling LLVM code itself.

Dec 16 2018, 8:20 AM · Restricted Project
ayonam updated the diff for D52707: Switch optimization in IR for known maximum switch values.

The earlier code had a problem in that if we converted every switch that had an unreachable default, to omit the check for the default values beyond the maximum switch case value given in the code, then there were some corner cases where the code generated was semantically incorrect and resulted in runtime failures. These failures are happening while compiling LLVM code itself.

Dec 16 2018, 7:36 AM · Restricted Project

Oct 23 2018

ayonam added inline comments to D52707: Switch optimization in IR for known maximum switch values.
Oct 23 2018, 5:30 AM · Restricted Project
ayonam added inline comments to D52707: Switch optimization in IR for known maximum switch values.
Oct 23 2018, 5:05 AM · Restricted Project

Oct 21 2018

ayonam updated the diff for D52707: Switch optimization in IR for known maximum switch values.

The earlier patch had a block of code that was commented out. This patch removes that block of code. All other changes asked for in the earlier review comments are there. Kindly review the same.

Oct 21 2018, 2:56 PM · Restricted Project
ayonam updated the diff for D52707: Switch optimization in IR for known maximum switch values.

I have updated the patch with the changes mentioned by Hans. Please review the same.

Oct 21 2018, 2:52 PM · Restricted Project
ayonam added inline comments to D52707: Switch optimization in IR for known maximum switch values.
Oct 21 2018, 2:48 PM · Restricted Project

Oct 10 2018

ayonam updated the diff for D52002: Omit range checks from jump tables when lowering switches with unreachable default.

I have made the changes that you mentioned.

Oct 10 2018, 11:15 AM · Restricted Project
ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

Sorry for the slow response.

This is looking promising. I have a few comments, and also this needs a good test.

Oct 10 2018, 8:38 AM · Restricted Project
ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

I have broken down the patch into two parts and this updated one is the first one. The second patch is under review ID D52707.

Oct 10 2018, 4:36 AM · Restricted Project
ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

A gentle reminder to the reviewers for comments on this revised patch.

Oct 10 2018, 4:33 AM · Restricted Project
ayonam updated the diff for D52707: Switch optimization in IR for known maximum switch values.

I have implemented the review comments given earlier. The updated patch is attached herewith. Kindly review the same.

Oct 10 2018, 4:12 AM · Restricted Project

Oct 2 2018

ayonam added inline comments to D52707: Switch optimization in IR for known maximum switch values.
Oct 2 2018, 2:32 PM · Restricted Project
ayonam added a comment to D52707: Switch optimization in IR for known maximum switch values.

The yellow bars are just Phabricator noting copy-pasted code; if you hover over the bar, you can get more details from the tooltip. You might not have to do anything about it in general, but in this case I think it's worth refactoring.

Oct 2 2018, 3:44 AM · Restricted Project

Sep 30 2018

ayonam added a comment to D52707: Switch optimization in IR for known maximum switch values.

What do those yellow lines on the side of the diff denote? Am I supposed to fix something there? Does that denote some kind of non-compliance to the coding standards that were detected?

Sep 30 2018, 11:29 PM · Restricted Project
ayonam created D52707: Switch optimization in IR for known maximum switch values.
Sep 30 2018, 11:26 PM · Restricted Project

Sep 28 2018

ayonam updated the diff for D52002: Omit range checks from jump tables when lowering switches with unreachable default.

In this update I have broken down the original patch into two parts and this is the first part, which handles the unreachable default. This part omits the generation of the conditional branch for checking the switch expression against the maximum value of the switch cases and jumping to the default block, if the default is unreachable.

Sep 28 2018, 9:24 AM · Restricted Project

Sep 18 2018

ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

The default is unreachable here because it would "fall off" the end of the function without returning, which is undefined behaviour. So the switch lowering should be able to remove the range check for the jump table.

On second thoughts, I realize that the default may not be unreachable in the case that I mentioned since the block beyond the switch block would then be treated as the default. Am I right?

Yes, if there are statements after the block, that would be treated as the default and it would not be unreachable.

Sep 18 2018, 3:13 AM · Restricted Project
ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

The default is unreachable here because it would "fall off" the end of the function without returning, which is undefined behaviour. So the switch lowering should be able to remove the range check for the jump table.

Sep 18 2018, 2:54 AM · Restricted Project
ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

If the default basic block is unreachable, that implies that one of the other cases must always be taken. Otherwise, the default wouldn't be unreachable after all.

For example:

int func(int x) {
    switch (x) {
    case 0: return f();
    case 1: return g();
    case 2: return h();
    case 3: return f();
    case 4: return h();
    }
}

The default is unreachable here because it would "fall off" the end of the function without returning, which is undefined behaviour. So the switch lowering should be able to remove the range check for the jump table.

Sep 18 2018, 2:41 AM · Restricted Project
ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

I will move the storing of the MaxSwitchValue to the metadata of the instruction. I think that is the right place to store these things.

If we expand the switch at the IR Level, the pass that does the transformation could just depend on whatever Analysis pass is necessary to determine the value range, and we wouldn't have to store it anywhere.

However, I beg to differ on the second point. If we do the changes in the IR by adding the requisite case statements, it will definitely generate the switch table that we expect to generate. However, when we mark the default branch as unreachable, the lowering code will redirect the unreachable default to the most popular switch block. And that would mean that the conditional branch for the default case would still be generated.

Right, we would have to address the FIXME first as discussed below.

Also, when you point to the FIXME in the visitSwitch() do you mean that we can completely do away with an unreachable default case? I feel theoretically we should be able to. If there is an unreachable default, then we should be able to jump to the end of the switch block. But even that would mean a conditional jump to the end of the switch block that would handle the default case.

No, the idea would be that if the default is unreachable, we can just omit the range check from the jump table completely.

SwitchToLookupTable in the SimplifyCFG pass already does the equivalent transformation: for switches used to select between constant values, it generates lookup tables, and if the default is unreachable it will omit the range check for the table completely.

I feel that the only way we can handle this is during lowering. That decision about whether to completely skip the branch for the default case is only possible at that level. Even if we do the creation of the expanded jump table by adding new case statements to the IR, this one thing probably has to be done at the time of lowering only. Would you have other suggestions to handle this aspect?

My suggestion is that the decision about whether to skip the branch for the default case should be driven by what the IR looks like: if the default basic block is unreachable, the branch can be skipped. I think this would be a clean design.

Sep 18 2018, 2:03 AM · Restricted Project

Sep 17 2018

ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

I don't think storing the MaxSwitchValue in the switch instructions itself is the right approach. Generally LLVM doesn't store analysis results in the instructions.

I also don't think the right way to do this is necessarily to change the jump table lowering itself, instead I'd suggest doing it at the IR level:

if we detect that the value range for the switch variable is small enough that adding a few more cases to the switch would cover the range completely, add those cases and mark the default bb unreachable. Then let the lowering code deal with exploiting the unreachable default bb. There is room for improvement there, see "FIXME: Exploit unreachable default more aggressively" in visitSwitch() but we should do that anyway.

What do you think?

Sep 17 2018, 5:10 AM · Restricted Project
ayonam added a comment to D52002: Omit range checks from jump tables when lowering switches with unreachable default.

I don't know enough about switch lowering to review this, so adding some more potential reviewers.
But there are at least a couple of obvious changes needed before this can proceed:

  1. The patch must include tests that show the difference with this patch.
  2. The indentation and spacing are clearly off (use clang-format?), and variable naming doesn't conform with: http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
Sep 17 2018, 4:06 AM · Restricted Project

Sep 12 2018

ayonam created D52002: Omit range checks from jump tables when lowering switches with unreachable default.
Sep 12 2018, 1:21 PM · Restricted Project