Page MenuHomePhabricator

delete the llvm.expect intrinsic and its lowering pass
AbandonedPublic

Authored by spatel on Apr 19 2016, 4:46 PM.

Details

Summary

This is the LLVM counterpart to:
http://reviews.llvm.org/D19299
Please see that patch for the motivation and more steps in this plan.

Diff Detail

Event Timeline

spatel updated this revision to Diff 54292.Apr 19 2016, 4:46 PM
spatel retitled this revision from to delete the llvm.expect intrinsic and its lowering pass.
spatel updated this object.
spatel added reviewers: hfinkel, davidxl, bkramer.
spatel added a subscriber: llvm-commits.
mgrang added a subscriber: mgrang.Apr 19 2016, 4:55 PM
mgrang added inline comments.
test/Assembler/auto_upgrade_intrinsics.ll
92

Extraneous newlines.

spatel updated this revision to Diff 54373.Apr 20 2016, 8:08 AM
spatel edited edge metadata.
spatel marked an inline comment as done.

Removed excess blank lines from test file.

davidxl added inline comments.Apr 21 2016, 12:47 PM
lib/IR/AutoUpgrade.cpp
664

Is there a reason not to upgrade it properly with meta data?

Another question for deprecation, can you just remove the documentation and leave the support for the intrinsic support for now?

spatel added inline comments.Apr 21 2016, 1:21 PM
lib/IR/AutoUpgrade.cpp
664

AFAIK, we don't make any guarantees to support legacy IR, except that we don't die processing it or miscompile it, so this is the minimal patch. Given that we're still losing metadata and/or not using it in a lot of cases (PR27344), I figured we should just focus on improving that instead of spending time on backwards support. If others disagree, I can add it.

If we just remove the docs, we'd still be doing unnecessary work for every compile, right? I don't see any reason to take that path.

davidxl added inline comments.Apr 21 2016, 1:30 PM
lib/IR/AutoUpgrade.cpp
664

Personally, I don't have issues with completely removing it -- but I can not speak for others in this aspect. IMO, removing documentation can be a safe first step -- the compile time impact should be minimal.

spatel added inline comments.Apr 21 2016, 2:17 PM
lib/IR/AutoUpgrade.cpp
664

Let me send a note to the dev list about this patch. If anyone objects or would like us to implement this in stages, we should hear about it. :)

spatel abandoned this revision.Apr 26 2016, 11:09 AM

Abandoning.

The feedback on the dev list was that handling the builtin_expect() in clang is ugly, so it's better to pay a small cost in LLVM to do it.

Note that the current llvm.expect lowering pass doesn't actually work for anything but the simplest cases. The pass needs to be enhanced to handle patterns like:

int foo(int x, int y) {
  if (__builtin_expect(x, 20) > 10) return 234;  // expected value is not 1
  return 2;
}

or:

int foo(int n) {
  int b = __builtin_expect(n, 1);  // expect is not directly used in comparison
  if (b) return 24;
  return 234;
}

Currently, the llvm.expect is discarded in these cases without generating any metadata.