llvm dev - Apr 2012 - [LLVMdev] [PATCH][RFC] Add llvm.codegen Intrinsic To Support Embedded LLVM IR Code Generation

On Apr 29, 2012, at 6:37 AM, Tobias Grosser wrote:
> 
> OK, I get what you mean. The intrinsic is currently targeted at the 
> OpenCL/CUDA model. It is the most widely used. Stuff like cell sounds 
> interesting, but probably needs further thoughts. Even with OpenCL/CUDA,
> this intrinsic works currently only for PTX code generation, but I hope 
> we can gain support for other GPU devices later on.
> 
>>    I agree that future work can be useful here. However, before
>>    spending a large amount of time to engineer a complex solution, I
>>    propose to start with the proposed light-weight approach. It is
>>    sufficient for our needs and will allow us to get the experience and
>>    infrastructure that can help us to choose and implement a more
>>    complex later on.
>> 
>> 
>> I agree that this approach is the best way to get short-term results,
>> especially for the GSoC project.
> 
> OK, let's go ahead.
> 
> Yabin, can you update the patch with the following changes:
> 
> - Remove the Arch flag
> - Document that we require a triple
> - Add two new arguments that take a feature string and a mcpu
>   flag (can be set to "", which means we use the default)
Wait. I don't think there is enough justification for this to move forward.
Apart from the technical issues that have already been raised. I can also see
this introduces a safety issue since the embedded IR code is not checked /
verified at compile time. Unless Chris says otherwise, I don't see this
patch being accepted on trunk.

Evan
> 
> Cheers
> Tobi
> 
> 
> 
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

On 04/29/2012 08:28 PM, Evan Cheng wrote:
>
> On Apr 29, 2012, at 6:37 AM, Tobias Grosser wrote:
>
>>
>> OK, I get what you mean. The intrinsic is currently targeted at the
>> OpenCL/CUDA model. It is the most widely used. Stuff like cell sounds
>> interesting, but probably needs further thoughts. Even with
OpenCL/CUDA,
>> this intrinsic works currently only for PTX code generation, but I hope
>> we can gain support for other GPU devices later on.
>>
>>>     I agree that future work can be useful here. However, before
>>>     spending a large amount of time to engineer a complex solution,
I
>>>     propose to start with the proposed light-weight approach. It is
>>>     sufficient for our needs and will allow us to get the
experience and
>>>     infrastructure that can help us to choose and implement a more
>>>     complex later on.
>>>
>>>
>>> I agree that this approach is the best way to get short-term
results,
>>> especially for the GSoC project.
>>
>> OK, let's go ahead.
>>
>> Yabin, can you update the patch with the following changes:
>>
>> - Remove the Arch flag
>> - Document that we require a triple
>> - Add two new arguments that take a feature string and a mcpu
>>    flag (can be set to "", which means we use the default)
>
> Wait. I don't think there is enough justification for this to move
forward. Apart from the technical issues that have already been raised. I can
also see this introduces a safety issue since the embedded IR code is not
checked / verified at compile time. Unless Chris says otherwise, I don't see
this patch being accepted on trunk.
Hi Even,

sure, this patch needs further discussion and an OK of some kind of 
global maintainer. Neither me or Justin can give permission for a change 
at this place. I mainly asked Yabin to provide a version that addresses 
concerns raised by Justin, to give further reviewers an up to date 
version to look at.

With your comment, you actually pointed out a bug.

Instead of:
Target->addPassesToEmitFile(PM, FOS,
                             TargetMachine::CGFT_AssemblyFile,
                             CodeGenOpt::Default)

We should use:
bool UseVerifier = true;
Target->addPassesToEmitFile(PM, FOS,
                             TargetMachine::CGFT_AssemblyFile,
                             UseVerifier)

Though, I don't think that is the problem you where talking about. Could 
you explain what security issues you exactly see? The embedded LLVM-IR 
is checked by the IR verifier the same as the host IR is. For both 
LLVM-IR modules the target code is generated and consequently verified 
at the same time. The embedded IR is _not_ compiled later than the host 
IR. What did I miss?

Cheers

>
> Evan
>
>>
>> Cheers
>> Tobi
>>
>>
>>
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>

Hi all,

I revised the llvm.codegen intrinsic according to all your comments.
Further discussion and review are welcome.
Main changes are list here.
1. remove the arg parameter.
2. add mcpu and features parameters.
3. Document that users of the intinsic should set the target triple in the
llvm IR string properly.
4. fix the bug of calling addPassesToEmitFile, using true for the verifier
flag.

Thanks a lot!

best regards,
Yabin

2012/4/30 Tobias Grosser <tobias at grosser.es>
> On 04/29/2012 08:28 PM, Evan Cheng wrote:
> >
> > On Apr 29, 2012, at 6:37 AM, Tobias Grosser wrote:
> >
> >>
> >> OK, I get what you mean. The intrinsic is currently targeted at
the
> >> OpenCL/CUDA model. It is the most widely used. Stuff like cell
sounds
> >> interesting, but probably needs further thoughts. Even with
OpenCL/CUDA,
> >> this intrinsic works currently only for PTX code generation, but I
hope
> >> we can gain support for other GPU devices later on.
> >>
> >>>     I agree that future work can be useful here. However,
before
> >>>     spending a large amount of time to engineer a complex
solution, I
> >>>     propose to start with the proposed light-weight approach.
It is
> >>>     sufficient for our needs and will allow us to get the
experience
> and
> >>>     infrastructure that can help us to choose and implement a
more
> >>>     complex later on.
> >>>
> >>>
> >>> I agree that this approach is the best way to get short-term
results,
> >>> especially for the GSoC project.
> >>
> >> OK, let's go ahead.
> >>
> >> Yabin, can you update the patch with the following changes:
> >>
> >> - Remove the Arch flag
> >> - Document that we require a triple
> >> - Add two new arguments that take a feature string and a mcpu
> >>    flag (can be set to "", which means we use the
default)
> >
> > Wait. I don't think there is enough justification for this to move
> forward. Apart from the technical issues that have already been raised. I
> can also see this introduces a safety issue since the embedded IR code is
> not checked / verified at compile time. Unless Chris says otherwise, I
> don't see this patch being accepted on trunk.
>
> Hi Even,
>
> sure, this patch needs further discussion and an OK of some kind of
> global maintainer. Neither me or Justin can give permission for a change
> at this place. I mainly asked Yabin to provide a version that addresses
> concerns raised by Justin, to give further reviewers an up to date
> version to look at.
>
> With your comment, you actually pointed out a bug.
>
> Instead of:
> Target->addPassesToEmitFile(PM, FOS,
>                             TargetMachine::CGFT_AssemblyFile,
>                             CodeGenOpt::Default)
>
> We should use:
> bool UseVerifier = true;
> Target->addPassesToEmitFile(PM, FOS,
>                             TargetMachine::CGFT_AssemblyFile,
>                             UseVerifier)
>
> Though, I don't think that is the problem you where talking about.
Could
> you explain what security issues you exactly see? The embedded LLVM-IR
> is checked by the IR verifier the same as the host IR is. For both
> LLVM-IR modules the target code is generated and consequently verified
> at the same time. The embedded IR is _not_ compiled later than the host
> IR. What did I miss?
>
> Cheers
>
>
> >
> > Evan
> >
> >>
> >> Cheers
> >> Tobi
> >>
> >>
> >>
> >>
> >>
> >> _______________________________________________
> >> LLVM Developers mailing list
> >> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> >
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120430/18f3936f/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-llvm.codegen-intrinsic.patch
Type: application/octet-stream
Size: 20368 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120430/18f3936f/attachment.obj>

On 04/29/2012 09:56 PM, Tobias Grosser wrote:
> On 04/29/2012 08:28 PM, Evan Cheng wrote:
>> Wait. I don't think there is enough justification for this to move
>> forward. Apart from the technical issues that have already been
>> raised. I can also see this introduces a safety issue since the
>> embedded IR code is not checked / verified at compile time. Unless
>> Chris says otherwise, I don't see this patch being accepted on
trunk.
[...]
> Could you explain what security issues you exactly see? The embedded
LLVM-IR
> is checked by the IR verifier the same as the host IR is. For both
> LLVM-IR modules the target code is generated and consequently verified
> at the same time. The embedded IR is _not_ compiled later than the host
> IR. What did I miss?
Hi Evan,

in your last mail, you pointed out security issues with this intrinsic.
Could you point them out to me?

Tobi