RFR(S): 8085932: Fixing bugs in detecting memory alignments in SuperWord

[Vladimir Kozlov]

Jan,

Here is why next code return false:

   if (_scale != 0) {
     return false;  // already found a scale

   if (_invar != NULL) return false; // already have an invariant


SWPointer() method tries to set _scale, _offset, _invar values.
But, for example, simple array access address uses 2 AddP nodes and each 
of them has offsets but different offsets. Usually one have invariant 
offset and another - scaled index:

   AddP (base, base, iv*scale + offset)
   AddP (base, addp, invar)

SWPointer() iterates over all AddP:

   for (int i = 0; i < 3; i++) {
     if (!scaled_iv_plus_offset(adr->in(AddPNode::Offset))) {
       assert(!valid(), "too complex");
       return;
     }
     adr = adr->in(AddPNode::Address);
     if (base == adr || !adr->is_AddP()) {
       break; // stop looking at addp's
     }
   }

And this code assumes only one of AddP can set those fields (_scale, 
_offset, _invar). If second AddP tries to set a field which is set by 
previous AddP it is considered complex address expression, for example:

   AddP (base, base, iv*scale + offset_con + invar1)
   AddP (base, addp, invar2)

and such cases are skipped.

Please, show your case for which you want to return 'true'.

Thanks,
Vladimir

On 6/18/15 5:10 PM, Vladimir Kozlov wrote:
> Thank you, Jan
>
> Fixes looks good but it would be nice if you replaced some tracing code
> with functions calls. In some place the execution code is hard to read
> because of big tracing code. For example, in
> SuperWord::memory_alignment() and in SWPointer methods.
>
> The one way to do that is to declare trace methods with empty body in
> product build, for example for SWPointer::scaled_iv_plus_offset() you
> may have new method declaration (not under #ifdef) in  superword.hpp:
>
>    class SWPointer VALUE_OBJ_CLASS_SPEC {
>
>      void trace_1_scaled_iv_plus_offset(...) PRODUCT_RETURN;
>
> and in superword.cpp you will put the method under ifdef:
>
> #ifndef PRODUCT
>    void trace_1_scaled_iv_plus_offset(...) {
>      ....
>    }
> #endif
>
> Then you can simply use it without ifdefs in code:
>
>   bool SWPointer::scaled_iv_plus_offset(Node* n) {
> +  trace_1_scaled_iv_plus_offset(...);
> +
>     if (scaled_iv(n)) {
>
> Note, macro PRODUCT_RETURN is defined as:
>
> #ifdef PRODUCT
> #define PRODUCT_RETURN  {}
> #else
> #define PRODUCT_RETURN  /*next token must be ;*/
> #endif
>
> Thanks,
> Vladimir
>
> On 6/8/15 9:15 AM, Civlin, Jan wrote:
>> Hi All,
>>
>>
>>   We would like to contribute to Fixing bugs in detecting memory
>>   alignments in SuperWord.
>>
>> The contribution Bug ID: 8085932.
>>
>> Please review this patch:
>>
>> Bug-id: https://bugs.openjdk.java.net/browse/JDK-8085932
>>
>> webrev: http://cr.openjdk.java.net/~kvn/8085932/webrev.00/
>>
>>
>>       *Description**: *Fixing bugs in detecting memory alignments in
>>       SuperWord
>>
>> Fixing bugs in detecting memory alignments in SuperWord:
>> SWPointer::scaled_iv_plus_offset (fixing here a bug in detection of
>> "scale"),
>> SWPointer::offset_plus_k (fixing here a bug in detection of "invariant"),
>>
>> Add tracing output to the code that deal with memory alignment. The
>> following routines are traceable:
>>
>> SWPointer::scaled_iv_plus_offset
>> SWPointer::offset_plus_k
>> SWPointer::scaled_iv,
>> WPointer::SWPointer,
>> SuperWord::memory_alignment
>>
>> Tracing is done only for NOT_PRODUCT. Currently tracing is controlled by
>> VectorizeDebug:
>>
>> #ifndef PRODUCT
>>    if (_phase->C->method() != NULL) {
>>         _phase->C->method()->has_option_value("VectorizeDebug",
>> _vector_loop_debug);
>>    }
>> #endif
>>
>> And VectorizeDebug may take any combination (bitwise OR) of the
>> following values:
>> bool is_trace_alignment() { return (_vector_loop_debug & 2) > 0; }
>> bool is_trace_mem_slice() { return (_vector_loop_debug & 4) > 0; }
>> bool is_trace_loop() { return (_vector_loop_debug & 8) > 0; }
>> bool is_trace_adjacent() { return (_vector_loop_debug & 16) > 0; }
>>