<div dir="ltr"><div class="im" style="font-family:arial,sans-serif;font-size:12.800000190734863px"> On 25/02/2014 23:51, Alp Toker wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>On 18/02/2014 10:59, Dmitri Gribenko wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">   I don't see any more obvious points for improvement, this patch LGTM, but it would be great if someone else took a quick look as well.<br>
</blockquote><br>LGTM as an evolution of the existing code. I'm guessing OPENMP_SIMD_CLAUSE is stubbed because you intend to fill it out in subsequent patches?<br><br>Longer term I think we should look to finding a better AST representation for OpenMP directives. There's an amount of do-nothing boilerplate going on here which could be handled automatically, say using synthesised AttributedStmts or folding the subclasses into a single Stmt node.<br>
</blockquote><br></div><div class="gmail_extra"><div>Hi Alp,</div><div><br></div><div>Thank you for review. </div><div><br></div><div>Yes, there are 7 clauses I plan to add for directive 'simd'<span style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:12.800000190734863px"> - safelen, linear, aligned, private, lastprivate, reduction and collapse.</span></div>
<div><br></div><div>Regarding folding different openmp directives into a single Stmt node in AST - we'll need to have some directive-specific fields in stmts - for example, expressions for loop counters calculation/update, which will be generated by Sema and then used in CodeGen. Such fields are needed (to be added) in OMPSimdDirective and would be useless in OMPParallelDirective, for example. Actually, when the directives/clauses behave similar, it should be possible to use same Stmt node with different DirectiveKind. But currently our approach is to have a different stmt for each directive - to keep them more decoupled from each other and from other parts of AST - and to use parent class like OMPExecutableDirective for decreasing amount of boilerplate-code a bit...</div>
<div><br></div><div>Best regards,</div><div>Alexander</div><div><br></div>
</div></div>