r225244 - Sema: analyze I,J,K,M,N,O constraints

[Joerg Sonnenberger]

On Sat, Jan 10, 2015 at 10:56:59PM -0800, Chandler Carruth wrote:
> On Sat, Jan 10, 2015 at 6:25 PM, Joerg Sonnenberger <joerg at britannica.bec.de
> > wrote:
> 
> > On Tue, Jan 06, 2015 at 04:26:34AM -0000, Saleem Abdulrasool wrote:
> > > Author: compnerd
> > > Date: Mon Jan  5 22:26:34 2015
> > > New Revision: 225244
> > >
> > > URL: http://llvm.org/viewvc/llvm-project?rev=225244&view=rev
> > > Log:
> > > Sema: analyze I,J,K,M,N,O constraints
> > >
> > > Add additional constraint checking for target specific behaviour for
> > inline
> > > assembly constraints.  We would previously silently let all arguments
> > through
> > > for these constraints.  In cases where the constraints were violated, we
> > could
> > > end up failing to select instructions and triggering assertions or worse,
> > > silently ignoring instructions.
> >
> > As discussion on IRC, I think the change is incorrect at least for 'K'.
> > It makes it impossible to encapsulate the equivalent to certain MMX/SSE
> > intrinsincs with always_inline functions, creating a regression for
> > pixman for example. We already have logic in the backend to ensure that
> > doesn't happen. In short: it is fine to verify that the constraint *can
> > be* be fulfilled, but it is not acceptable to require immediates in
> > cases where the inliner or GVN would be able to compute constant
> > arguments later.
> 
> 
> FWIW, I disagree.
> 
> Accepting or rejecting code should be stable across optimizations. If Clang
> can't reliably fold the argument to a constant, I think it is important to
> reject it.

There is a related issues here, one is that FastIsel is extremely
badd when it comes to handling of immediates, but that I don't think it
is valid to use arbitrary restrictions in one part of LLVM to justify
incorrect design choices in another.

> This is not the first time that Clang has disagreed with GCC on this
> specific front -- we also refuse to use the optimizer to implement
> __builtin_constant_p because it would produce unpredictable results and
> violates fundamental layering design decisions in LLVM and Clang.

Interesting argument, given that __builtin_constant_p *has* been changed
to be expanded later.

> Fortunately, pixman is the only example of code I have seen that was
> regressed by this, and the code in pixman I think can even fail with GCC
> under the right circumstances, so I don't see this as a problem.

Not true, variants like the attached code have been in use for a while.
In fact, my clang 3.5.0 build does the correct thing here where ToT now
complains.

Joerg
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test.c
Type: text/x-csrc
Size: 385 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150111/8f263bce/attachment.c>