[llvm-commits] Invalid instcombine transformation

[Chris Lattner]

On Nov 26, 2009, at 12:25 PM, Anton Korobeynikov wrote:
> Hello, Everyone
> 
> Currently instcombine turns small memcpy's (say, of 1/2/4/8 bytes)
> into loads / stores. 

Yes, this is very bad.  instcombine should not 'lower' into 2/4/8 byte stores (1 byte are ok though).  This is for a couple of reasons, not the least of which is that it introduces type-unsafe code that is harder to analyze than the original IR.

> This seems to be invalid if target does not allow
> unaligned access (and even if it allows, it's slower in general).

The target *has* to support unaligned accesses.  Worst case, the code generator should lower to byte accesses.  That said, this is still bad :)

> Attached patch allows memcpy => load/store transformation only if memcpy
> alignment is not lower than ABI alignment of the load/store type (when
> TargetData is available).
> 
> This fixed several bugs in real code running on ARM.

I'd prefer to completely remove this from instcombine.  To do this, please verify that we are already doing the equivalent lowering in codegen (e.g. on x86 a 2 and 4-byte memset should turn into an unaligned store).  Then verify that disabling the instcombine xform doesn't cause any significant changes in codegen on multisource (or something else signifcant).

-Chris