Request for review (M): 7029017 Additional architecture support for c2 compiler

[Vladimir Kozlov]

Here is another suggestion for you. Use final_graph_reshaping to mask shift 
count (in final_graph_reshaping_impl()):

   case Op_LShiftI:
   case Op_RShiftI:
   case Op_URShiftI:
   case Op_LShiftL:
   case Op_RShiftL:
   case Op_URShiftL:
     if (Matcher::need_masked_shift_count) {
       Node* in2 = n->in(2);
       juint mask = (n->bottom_type() == TypeInt::INT) ? (BitsPerInt - 1) : 
(BitsPerLong - 1);
       const TypeInt* t = in2->find_int_type();
       if (t != NULL && t->is_con()) {
         juint shift = t->get_con();
         if (shift > mask) { // Unsigned cmp
           Compile* C = Compile::current();
           n->set_req(2, ConNode::make(C, TypeInt::make(shift & mask)));
         }
       } else {
         if (t == NULL || t->_lo < 0 || t->_hi > mask) {
           Compile* C = Compile::current();
           Node* shift = new (C, 3) AndINode(in2, ConNode::make(C, 
TypeInt::make(mask)));
           n->set_req(2, shift);
         }
       }
       if (in2->outcnt() == 0) { // Remove dead node
         in2->disconnect_inputs(NULL);
       }
     }
     break;

Vladimir

Vladimir Kozlov wrote:
> What I meant is:
> 
> in your platform .ad file you asign 'true' and in others 'false':
> 
> const bool Matcher::need_masked_shift_count = true;
> 
> in matchser.hpp:
> 
> + static const bool need_masked_shift_count;
> 
> in mulnode.cpp:
> 
> +  if (Matcher::need_masked_shift_count) {
> 
> 
> I am concern about masking during ideal transformation. It affects all 
> platforms and you will convert a negative value to positive which may 
> produce a different result since it may affect some ideal 
> transformations. I would suggest to do it in .ad file.
> 
> instruct sarI_reg_imm(iRegI dst, iRegI src1, immI src2) %{
>   match(Set dst (RShiftI src1 src2));
>   format %{ "SRA    $src1,$src2,$dst" %}
>   ins_encode %{
>     int shift = ($src2$$constant) & (BitsPerInt - 1);
>     __ sra($src1$$Register, shift, $dst$$Register);
>   %}
>   ins_pipe(ialu_reg_imm);
> %}
> 
> 
> And for variable shift you can use predicate in .ad file to avoid AND:
> 
> // Version without AND
> instruct sarI_reg_reg(iRegI dst, iRegI src1, iRegI src2) %{
>   predicate(n->in(2)->is_shift(BitsPerInt - 1));
> 
> // Version with AND
> instruct sarI_reg_reg(iRegI dst, iRegI src1, iRegI src2) %{
>   predicate(!n->in(2)->is_shift(BitsPerInt - 1));
>   match(Set dst (RShiftI src1 src2));
> 
> and in node.hpp
> 
> is_shift(int mask) {
>   const TypeInt* t = find_int_type();
>   return (t != NULL) && t->_lo >= 0 && t->_hi <= mask;
> }
> 
> 
> Here are few comments for future C2 code modification.
> Don't do assignment in the condition code:
> 
> +    const TypeInt *t;
> +    // avoid infinite loop of shift transformations
> +    if( !(t = phase->type(in2)->isa_int()) || t->_lo < 0 || t->_hi > m ) {
> 
> Use "if (a)" spacing instead of "if( a )".
> 
> Also you should not modify dead node, it is useless:
> 
> -  if( t == Type::TOP ) return NULL;       // Right input is dead
> +  if( t == Type::TOP ) return do_mask_shift(phase, BitsPerInt - 1); // 
> Right input is dead
> 
> 
> Vladimir
> 
> Roland Westrelin wrote:
>> Hi Vladimir,
>>
>>> Did you consider to use Matcher property instead of new flag and new 
>>> ideal node
>>> code? See how we do Matcher::clone_shift_expressions.
>>
>> I would like the extra and operations to be optimized out when 
>> possible. That won't happen if I do the work at the matcher level, right?
>>
>> Roland.