Page MenuHomePhabricator

[CodeGen] Compute DT/LI lazily in SafeStackLegacyPass. NFC.
ClosedPublic

Authored by ab on Mar 23 2017, 1:17 PM.

Details

Summary

We currently require SCEV, which requires DT/LI. Those are expensive to compute, but the pass only runs for functions that have the safestack attribute.
Compute DT/LI to build SCEV lazily, only when the pass is actually going to transform the function.

This updates a test I added in my local branch, that checks the entire O0 llc x86 pipeline. It's a little overzealous, but hopefully that makes changes to the O0 pipeline obvious; it's easy to require expensive passes, and this helps make informed decisions.

Diff Detail

Repository
rL LLVM

Event Timeline

ab created this revision.Mar 23 2017, 1:17 PM
davide added a subscriber: davide.Mar 30 2017, 1:17 PM

Do you have #s on the compile time impact? I'd rather err on the side of safety (i.e. recomputing each time) unless the impact is substantial (I assume it is, but I'm just curious and I'd like to see the #s).

ab added a subscriber: mzolotukhin.Apr 3 2017, 7:27 PM

Of course, sorry, here's some context: Michael and I noticed 4 unnecessary DT computations at O0. Altogether, they added up to a non-trivial compile-time win on the CTMark subset of the test-suite: 1.3-4.7%, 2.5% geomean.

This removes one of the 4 DT uses. Here are patches for the other two: D31641, D31642.

I think this is awkward, but it seems like a reasonably self-contained stopgap until we have cached lazy analyzes. (the least bad alternative I came up with was to add a TargetOption for safestack, and disable it from clang if we weren't passed -fsanitize=safestack. But that has a bunch of edge cases we might as well avoid, for llc, LTO, and clang on IR inputs).

To be clear, I don't think this is acceptable if it becomes pervasive: it seems like these backend passes are exceptions in that they're part of the O0 pipeline, only run at a fixed location in the pipeline, and are known to follow other pre-isel passes that don't preserve anything. More importantly, we have a cleaner path forward with the new pass manager.

FWIW, I'm pretty happy with this approach especially for something triggered by an attribute like this. It is a bit gross, but doesn't seem *that* gross. Still, should see if your explanation satisfies davide. =]

davide accepted this revision.Apr 12 2017, 12:03 PM

LGTM

This revision is now accepted and ready to land.Apr 12 2017, 12:03 PM
pcc accepted this revision.Apr 12 2017, 12:04 PM

LGTM as well

ab added a comment.Apr 13 2017, 2:05 AM

Thanks folks! No objections for D31303 then?

This revision was automatically updated to reflect the committed changes.