Use Intrinsic::getDeclaration to obtain the nacl_read_tp intrinsic

The customary LLVM way of obtaining intrinsics is with
    Intrinsic::getDeclaration and use the definition in
    include/llvm/Intrinsics.td
    
    This also makes the attribute on the intrinsic to be more
    consistent with the back-end (code-gen), which automatically assumes
    it's ReadNone (because this is what Intrinsics.td) defines.
    
    Using ReadNone rather than ReadOnly may be not strictly correct because
    the intrinsic depends on the value of the TP. However, this attribute is
    not really used anywhere in IR optimizations, and in the backend the
    intrinsic is ReadNone anyhow (the IR setting gets overridden).
    
    If we run into any problems with this in the future, we may consider
    handling the lowering of this intrinsic in
    TargetLowering::LowerINTRINSIC_W_CHAIN rather than in
    TargetLowering::LowerINTRINSIC_WO_CHAIN.

BUG=None
R=mseaborn@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=8fe6f1f

[eliben, 2013-05-14 20:04:23]

Hi Mark,

While working on the setjmp-rewrite pass, I noticed that the ExpandTls pass uses
a long-winded way to declare and obtain the llvm.nacl.read.tp intrinsic, while
the customary way in LLVM is to use the definition in
include/llvm/IR/Intrinsics.td and then use Intrinsic::getDeclaration (the former
method is only used in ExpandTls when grepping globally on LLVM, the latter is
used in many places).

This CL switches to Intrinsic::getDeclaration; all tests pass.

Note that there's one difference: the existing code explicitly set the
Attribute:ReadOnly attribute on the intrinsic, while it's actually defined in
Intrinsics.td as ReadNone. The latter seems to be correct while being more
strict. However, I may be missing something here so PTAL.

Also, I removed the GCCBuiltin<"__builtin_nacl_read_tp"> thing from the
intrinsic definition; I don't thin these intrinsics are exposed to user code as
builtins?

[Mark Seaborn, 2013-05-14 21:10:05]

On 14 May 2013 13:04, <eliben@chromium.org> wrote:

> Reviewers: Mark Seaborn,
>
> Message:
> Hi Mark,
>
> While working on the setjmp-rewrite pass, I noticed that the ExpandTls
> pass uses
> a long-winded way to declare and obtain the llvm.nacl.read.tp intrinsic,
> while
> the customary way in LLVM is to use the definition in
> include/llvm/IR/Intrinsics.td and then use Intrinsic::getDeclaration (the
> former
> method is only used in ExpandTls when grepping globally on LLVM, the
> latter is
> used in many places).
>
> This CL switches to Intrinsic::getDeclaration; all tests pass.
>
> Note that there's one difference: the existing code explicitly set the
> Attribute:ReadOnly attribute on the intrinsic, while it's actually defined
> in
> Intrinsics.td as ReadNone. The latter seems to be correct while being more
> strict. However, I may be missing something here so PTAL.
>
> Also, I removed the GCCBuiltin<"__builtin_nacl_**read_tp"> thing from the
> intrinsic definition; I don't thin these intrinsics are exposed to user
> code as
> builtins?
>

LGTM.  Please put some of that description into the commit message.

BTW, ReadNone isn't correct for this intrinsic because ReadNone means "This
attribute indicates that the function computes its result (or decides to
unwind an exception) based strictly on its arguments, without... accessing
any mutable state (e.g. memory, control registers, etc) visible to caller
functions".  But accessing a "control register" (the thread's thread
pointer) is exactly what this intrinsic does.  The thread pointer is
settable by the pexe via nacl_tls_init() IRT calls.

However, your change doesn't affect whether ReadNone gets applied in
practice, AFAIK.  To be on the safe side, make sure you do a full run of
the tests before committing, to check that the nacl.read.tp() call isn't
hoisted to before a pexe's nacl_tls_init() call.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[eliben, 2013-05-14 22:06:47]

Committed patchset #2 manually as r8fe6f1f (presubmit successful).

[Mark Seaborn, 2013-05-15 22:05:07]

On 14 May 2013 13:04, <eliben@chromium.org> wrote:

> While working on the setjmp-rewrite pass, I noticed that the ExpandTls
> pass uses
> a long-winded way to declare and obtain the llvm.nacl.read.tp intrinsic,
> while
> the customary way in LLVM is to use the definition in
> include/llvm/IR/Intrinsics.td and then use Intrinsic::getDeclaration (the
> former
> method is only used in ExpandTls when grepping globally on LLVM, the
> latter is
> used in many places).
>

BTW, if Module::getOrInsertTargetIntrinsic() isn't used or tested upstream,
maybe it should be removed upstream?

I think the reason I didn't find Intrinsic::getDeclaration() is that
Intrinsic is a namespace and not a class, and I've mostly been using the
class list in LLVM's Doxygen docs to navigate around (
http://llvm.org/docs/doxygen/html/annotated.html).

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[eliben, 2013-05-15 22:44:54]

On 2013/05/15 22:05:07, Mark Seaborn wrote:
> On 14 May 2013 13:04, <mailto:eliben@chromium.org> wrote:
> 
> > While working on the setjmp-rewrite pass, I noticed that the ExpandTls
> > pass uses
> > a long-winded way to declare and obtain the llvm.nacl.read.tp intrinsic,
> > while
> > the customary way in LLVM is to use the definition in
> > include/llvm/IR/Intrinsics.td and then use Intrinsic::getDeclaration (the
> > former
> > method is only used in ExpandTls when grepping globally on LLVM, the
> > latter is
> > used in many places).
> >
> 
> BTW, if Module::getOrInsertTargetIntrinsic() isn't used or tested upstream,
> maybe it should be removed upstream?

Good point. Done in r181943

> 
> I think the reason I didn't find Intrinsic::getDeclaration() is that
> Intrinsic is a namespace and not a class, and I've mostly been using the
> class list in LLVM's Doxygen docs to navigate around (
> http://llvm.org/docs/doxygen/html/annotated.html).
> 
> Cheers,
> Mark
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:native-client-reviews+unsubscribe@googlegroups.com.
> To post to this group, send email to
mailto:native-client-reviews@googlegroups.com.
> Visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.
> For more options, visit https://groups.google.com/groups/opt_out.
> 
>