[llvm-commits] [llvm] r75571 - in /llvm/trunk: lib/Transforms/Scalar/SimplifyCFGPass.cpp test/Transforms/SimplifyCFG/2009-07-13-no-delete-asm.ll

[Dale Johannesen]

On Jul 13, 2009, at 9:03 PMPDT, Chris Lattner wrote:

>
> On Jul 13, 2009, at 5:53 PM, Eli Friedman wrote:
>
>> On Mon, Jul 13, 2009 at 5:45 PM, Dale Johannesen<dalej at apple.com>  
>> wrote:
>>> --- llvm/trunk/test/Transforms/SimplifyCFG/2009-07-13-no-delete- 
>>> asm.ll (added)
>>> +++ llvm/trunk/test/Transforms/SimplifyCFG/2009-07-13-no-delete- 
>>> asm.ll Mon Jul 13 19:45:38 2009
>>> @@ -0,0 +1,11 @@
>>> +; RUN: llvm-as < %s | opt -simplifycfg | llvm-dis | grep xor
>>> +; ModuleID = '<stdin>'
>>> +target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16- 
>>> i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128- 
>>> a0:0:64-f80:128:128"
>>> +target triple = "i386-apple-darwin9.6"
>>> +
>>> +define void @bar() nounwind {
>>> +entry:
>>> +       store i16 undef, i16* null
>>> +       %asmtmp = call i32 asm sideeffect "xor $0, $0",  
>>> "=={bx},rm,~{dirflag},~{fpsr},~{flags},~{memory}"(i16 undef)  
>>> nounwind           ; <i32> [#uses=0]
>>> +       ret void
>>> +}
>>
>> Umm, this testcase quite clearly has undefined behavior; what is this
>> patch trying to fix?
>
> Beyond that, what does the store have to do with the undef.  The  
> store being undefined means the asm is not reachable, so it should  
> be deleted.

No, the gcc doc is (for once) quite clear that volatile asm's should  
never be deleted.  They aren't necessarily executable code, they might  
change the section of assembly, or define a global data object, or  
something like that.  I could probably be persuaded to add a check for  
volatile, but I don't think it's a good idea; the goal of asm handling  
should be to imitate gcc as much as possible (that's the only  
reasonable definition of how they should work, although it's probably  
not fully implementable in llvm), and gcc doesn't eliminate this one.

The original testcase, which I perhaps should not have modified,  
involves a load from undefined memory feeding into a volatile asm;  
earlier optimizations removed the load in favor of the store above.    
They probably shouldn't have done that, which is a later patch.

Asm's are outside standards, so standard concepts like "undefined  
behavior" aren't very useful in dealing with them.