[polly] r271513 - Simplify the type adjustment in the IslExprBuilder

[Tobias Grosser via llvm-commits]

On 06/08/2016 10:52 AM, Johannes Doerfert wrote:
> On 06/07, Tobias Grosser wrote:
>> On 06/02/2016 01:16 PM, Johannes Doerfert via llvm-commits wrote:
>>> Author: jdoerfert
>>> Date: Thu Jun  2 06:15:57 2016
>>> New Revision: 271513
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=271513&view=rev
>>> Log:
>>> Simplify the type adjustment in the IslExprBuilder
>>>
>>>   We now have a simple function to adjust/unify the types of two (or three)
>>>   operands before an operation that requieres the same type for all operands.
>>>   Due to this change we will not promote parameters that are added to i64
>>>   anymore if that is not needed.
>>
>> Hi Johannes,
>>
>> this patch seems to mix a larger no-functional-change refactoring with a
>> change that reduces the size of types under certain conditions.
> This is not true as there is no larger fundamental change.

I did not mean that this is a larger fundamental change.

I only meant this is a more-than-100-line refactoring (which is mostly
mechanic, I suppose) mixed with a change of type sizes.

>> Unfortunately the commit message does not explain why using smaller
>> types is "safe" and it also becomes not clear from the code. In fact,
>> our current code does not have functionality to derive a 'save' type.
>> i64 just happens to be a little saver than i32, which is why we
>> aggressively promote to i64.
> There is no comment explaining a lot because not much changed.
> Everything that changed is explained in the commit message. If you
> disagree please point out something more specific.

To me it remains unclear why smaller types provide the same safety
guarantees as the 64 bit types we used before. 64 bit types have been
chosen very deliberately. There may be smaller/better types to use, but
I would prefer to discuss and understand this before we go ahead.

>> As discussed on the phone there are different approaches to ensure
>> safety, but we probably should have the design discussion before we
>> start to play with the width of the type. Can we possibly revert this
>> patch for now until this discussion has taken place. Otherwise, it is
>> difficult to see what has changed or not.
> This patch does not necessarily change the type of the expressions,
> especially not in combination with 97b7a14. If yo disagree please point
> out a change in the generated types.

According to your commit message the type of expressions may change:

"Due to this change we will not promote parameters that are added to i64
anymore if that is not needed."

Also some of the test cases have changed types:

-; IR-NEXT:   %.obit7 = extractvalue { i64, i1 } %21, 1
+; IR-NEXT:   %.obit7 = extractvalue { i32, i1 } %16, 1


I reverted this and some of the follow up commits in r272483 as we
already started to build on this. In case we agree that this is the
right direction, we should be able to re-commit these changes with
little effort.

As reverts sometimes cause confusion, I would like to clarify that this
revert is not a judgment over the quality of the code that is reverted,
but is just the LLVM approach of how to react in case some design
decisions are still open [1].

Best,
Tobias

[1] http://lists.llvm.org/pipermail/llvm-dev/2016-March/096529.html