This is an archive of the discontinued LLVM Phabricator instance.

[clang] Mark libm functions writeonly when we care about errno
AcceptedPublic

Authored by homerdin on Jun 20 2018, 12:05 PM.

Details

Summary

This patch as is depends on the addition of a write_only attribute in clang: https://reviews.llvm.org/D46313

This patch will add the write_only attribute to libm functions when we care about errno, with the intent of eventually enabling CSE and Hoisting of WriteOnly functions when it is safe to do so.

The LLVM WriteOnly attribute could be added directly without adding a write_only, though it seems beneficial to expose the attribute to users.

Diff Detail

Event Timeline

homerdin created this revision.Jun 20 2018, 12:05 PM

Basic approach seems reasonable to me.

lib/Sema/SemaDecl.cpp
13483

Can we reasonably unify these three blocks? Like maybe:

static auto * const isKnownNotToChangeErrorOnTarget = [](ASTContext &Context, unsigned BuiltinID) {
  auto &triple = Context.getTargetInfo().getTriple();
  switch (BuiltinID) {
  case ...:
    return triple.isGNUEnvironment() || triple.isAndroid() || triple.isOSMSVCRT();
  default:
    return false;
  }
};

if (!FD->hasAttr<ConstAttr>() && Context.BuiltinInfo.isConstWithoutErrno(BuiltinID)) {
  if (!getLangOpts().MathErrno || isKnownNotToChangeErrnoOnTarget(Context, BuiltinID)) {
    FD->addAttr(ConstAttr::CreateImplicit(Context, FD->getLocation()));
  } else {
    FD->addAttr(WriteOnlyFuncAttr::CreateImplicit(Context, FD->getLocation()));
  }
}
rjmccall added inline comments.Jun 21 2018, 8:24 PM
lib/Sema/SemaDecl.cpp
13483

I was really overthinking the declaration of isKnownNotToChangeErrorOnTarget; it should just be an auto and non-static.

homerdin updated this revision to Diff 157371.Jul 25 2018, 3:02 PM

Unified the blocks that add WriteOnly and Const attributes

hfinkel accepted this revision.Aug 13 2018, 3:20 PM

LGTM too.

This revision is now accepted and ready to land.Aug 13 2018, 3:20 PM
homerdin updated this revision to Diff 163173.Aug 29 2018, 1:40 PM
homerdin edited the summary of this revision. (Show Details)

Rebased and updated to use new name for WriteOnly attribute.

Also added test/CodeGen/libcall-declarations.c update which I had not included in the previous diff.