[LLVMdev] Overzealous PromoteCastOfAllocation

[Dan Gohman]

On Sep 23, 2008, at 4:16 AM, Matthijs Kooijman wrote:
>
>> and then does a load or store at the higher alignment, is
>> invoking undefined behavior. The alignment attribute on a load or  
>> store
>> is an assertion about the actual alignment of the memory.
> Should this undefined behaviour be caught by the verifier, or is it  
> ok for it
> to exist?

I don't think so. It's not the Verifier's job to diagnose undefined
behavior. The IR in this case is valid, so the verifier should
accept it.

For better or worse, stores to null pointers are pretty common in
LLVM IR. The most common source of these is unreachable code, and
bugpoint, but those are important use cases :-).

However, it might be interesting to have a separate
undefined-behavior-detector pass.  An LLVM pass wouldn't likely be
usable as an user-level bug finder; clang's static analysis is
much better positioned to do that kind of thing. But an LLVM pass
would be able to find bugs in front-ends and optimization passes.

> In any case, since this behaviour is undefined, it's not quite  
> needed for the
> instcombine pass to propagate the ABI alignment from a type  
> bitcasted to
> upwards into the alloca instruction, as you suggested. The frontend  
> generating
> the IR should have done this already when needed (when a load with  
> that higher
> alignment uses the bitcasted value, or the bitcasted value is passed  
> to
> another function, which requires ABI alignment).

Your analysis sounds correct. I misunderstood the situation before.

> I've now created a patch that makes sure that any struct typed  
> alloca is never
> changed by instcombine, unless all it's uses are bitcasts to the  
> same type.
> This seems like the most sane way to do things. This change did  
> expose some
> alignment-related bug in llvm-gcc, see PR2823. This means that  
> committing
> preserve-struct.diff will result in a test-suite failure (at least
> SingleSource/UnitTests/Vector/divides, I haven't finished testing  
> Multisource
> yet), but this is caused by llvm-gcc, not instcombine.
>
> Related to this issue, I've created two more patches. I'm including  
> them here
> for review, comments are welcome. I'm not sure if these patches  
> actually make
> a lot of difference in real world applications and they are not yet  
> sufficient
> to make scalarrepl do its work completely in our example code (lots  
> of struct
> passing and the resulting memmoves), but they will at least make the  
> resulting
> code more logical and make it possible to get there.
>
> I've tested these patches against the dejagnu tests and the test- 
> suite,
> finding only two failures. The first is the one regarding PR2823  
> referenced
> above. The second one is concerning
> MultiSource/Benchmarks/MiBench/security-blowfish, for some reason  
> opt emits an
> invalid bitcode file (the bitcode reader asserts). From playing with  
> the
> debugger, it seems a store instruction is asked to store a an i8* to  
> an
> [8 x i8]*. However, since the verifier doesn't catch this before  
> outputting
> the bitcode, it's probably something more involved. I'll look into  
> this issue
> a bit closer later (currently I don't even know which of the three  
> patches
> causes it).
>
> Anyway, review of these patches and opinions as to their usefulness  
> would be
> welcome!


I haven't read your patches yet, but it sounds like you're heading in a
positive direction here :-).

Dan