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

Tobias Grosser via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 7 09:26:16 PDT 2016


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.

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.

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.

Best,
Tobias

PS: I am very sorry to ask for a revert, but this really touches a
fundamental parts of the AST generation and it is a lot easier to reason
about such changes ahead of commit, rather then having the
noise of changing designs in the code base.


More information about the llvm-commits mailing list