[Mesa-dev] [PATCH 1/3] clover: separate compile and link stages

[Francisco Jerez]

EdB <edb+mesa at sigluy.net> writes:

> On Sunday 05 July 2015 18:15:33 Francisco Jerez wrote:
>>[...]
>> > --- a/src/gallium/state_trackers/clover/core/error.hpp
>> > +++ b/src/gallium/state_trackers/clover/core/error.hpp
>> > @@ -68,10 +68,31 @@ namespace clover {
>> > 
>> >     class build_error : public error {
>> >     
>> >     public:
>> >        build_error(const std::string &what = "") :
>> > +         error(CL_BUILD_PROGRAM_FAILURE, what) {
>> > +      }
>> > +   };
>> > +
>> 
>> This exception class now seems redundant -- With program::build() gone
>> build is no longer a thing.
>
> It's still needed by tgsi.
> I plan to rework this part later to make it consistent with the way it's 
> handle in llvm/invocation but first off I wanted to be done with clLink :/
>
The tgsi path could also throw compile_error AFAICT?

>> 
>> > +   class compile_error : public error {
>> > +   public:
>> > 
>> > +      compile_error(const std::string &what = "") :
>> >           error(CL_COMPILE_PROGRAM_FAILURE, what) {
>> >        
>> >        }
>> >     
>> >     };
>> > 
>> > +   class link_error : public error {
>> > +   public:
>> > +      link_error(const std::string &what = "") :
>> > +         error(CL_LINK_PROGRAM_FAILURE, what) {
>> > +      }
>> > +   };
>> > +
>> > +   class link_option_error : public error {
>> > +   public:
>> > +      link_option_error(const std::string &what = "") :
>> > +         error(CL_INVALID_LINKER_OPTIONS , what) {
>> > +      }
>> > +   };
>> > +
>> 
>> I don't think you really need to special-case link_option_error against
>> the less specific clover::error class?
>
> clLinkProgram should not create a program if it failed to parse the given 
> options, I will use this class to handle this case. Other case should create 
> the said program.
> That said, it could also have been created in later patch.
>

Ah, fair enough.

>> 
>> >     template<typename O>
>> >     class invalid_object_error;
>> > 
>> > diff --git a/src/gallium/state_trackers/clover/core/program.cpp
>> > b/src/gallium/state_trackers/clover/core/program.cpp index
>> > 0d6cc40..21faf4e 100644
>> > --- a/src/gallium/state_trackers/clover/core/program.cpp
>> > +++ b/src/gallium/state_trackers/clover/core/program.cpp
>>[...]
>> 
>> > +      std::string log;
>> > +
>> > +      try {
>> > +         auto module = link_program_llvm(mods,
>> > +                                         dev.ir_format(),
>> > dev.ir_target(),
>> > +                                         opts, log);
>> > +         _binaries.insert({ &dev, module });
>> > +         append_to_log(&dev, log);
>> > +      } catch (const link_option_error &) {
>> > +         append_to_log(&dev, log);
>> > +         throw;
>> > +      } catch (const error &) {
>> > +         append_to_log(&dev, log);
>> > +         r = false;
>> 
>> I suggest you just catch "const error &", update the error log and
>> rethrow here, so you save a catch block and an error subclass.
>
> As explain clLinkProgram doesn't behave the same way regarding error during 
> option parsing and after.
>
Still I doubt that you need to handle them separately here, just update
the log and rethrow whatever you got, clLinkProgram can still give
link_error special treatment and return the created program despite the
failure for the application to be able to read back the error log.

>[...]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150705/b057e248/attachment.sig>