HomePhabricator

[LSR] Tweak setup cost depth threshold to 10.

Description

[LSR] Tweak setup cost depth threshold to 10.

The original change introduced a depth limit of 7 which caused a 22% regression
in the Swift MapReduceLazyCollection & Ackermann benchmarks. This new threshold
still ensures that the original test case doesn't hang.

rdar://50359639

Details

Committed
aemersonMay 10 2019, 10:29 AM
Parents
rL360443: Finish renaming CompileUnit -> Unit
Branches
Unknown
Tags
Unknown

Event Timeline

We are changing one magic number to another magic number here, and no test was added. If I'm right, the commit message mentions only 1 benchmark? This all together, feels like a big hack to me.

Is there a better approach to this magic number?

Slightly off topic, I also don't see the point of adding rdar tags to commit messages as they are meaningless to non-Apple folks.

Fair enough, I've reverted this in r360589. The test case seemed cumbersome to reduce and commit just for a threshold change, but I'm not sure what the best approach is here yet beyond a depth limit change.

As for rdar, that's been accepted in LLVM for a long time in commit messages, as long as they're not in code comments.

We are changing one magic number to another magic number here, and no test was added. If I'm right, the commit message mentions only 1 benchmark? This all together, feels like a big hack to me.

Is there a better approach to this magic number?

Slightly off topic, I also don't see the point of adding rdar tags to commit messages as they are meaningless to non-Apple folks.

Fair enough, I've reverted this in r360589. The test case seemed cumbersome to reduce and commit just for a threshold change, but I'm not sure what the best approach is here yet beyond a depth limit change.

Mmm, okay. To be honest, I also don't know what the right approach is here, and am not sure a revert was necessary. But from a quick glance at our results this morning, there were quite some changes, positive and negative, but mostly down overall.

From what I understand, this SetupCostDepthLimit is a bit of a funny one, because it says something about the cost of instructions outside the loop. If changes in this magic number causes big differences in performance, then probably something funny is going on with the modeling of instructions inside the loop. I guess this means we need to understand what exactly is going on here, but the fact we can have these big swings in performance is perhaps not really a good sign.

As for rdar, that's been accepted in LLVM for a long time in commit messages, as long as they're not in code comments.

Okay, fair enough, although I see absolutely no value for this to others in the community.