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

[Chris Lattner]

On Jul 14, 2009, at 10:09 AM, Dale Johannesen wrote:

>
> 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.

You are probably thinking of a file scope asm block.  volatile asms  
absolutely can be duplicated (thus they can't define global data) and  
can definitely be deleted.

>
> 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.

It sounds like the problem is the earlier pass, please revert r75571.

-Chris