llvm dev - May 2008 - [LLVMdev] More info, was Help needed after hiatus

Hi,

I know my last question was very vague (i.e. "It stopped working, what 
went wrong?"), so here is a little more concrete example:

If I run the optimizer (opt) on this code snippet with -std-compile-opts 
the optimizer hangs.


; ModuleID = 'test.ubc'
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-s0:0:64-f80:32:32"
target triple = "i686-pc-linux-gnu"

declare void @BZALLOC(i32)

define void @f(i32) {
entry:
         %blockSize100k = alloca i32             ; <i32*> [#uses=2]
         store i32 %0, i32* %blockSize100k
         %n = alloca i32         ; <i32*> [#uses=2]
         load i32* %blockSize100k                ; <i32>:1 [#uses=1]
         store i32 %1, i32* %n
         load i32* %n            ; <i32>:2 [#uses=1]
         add i32 %2, 2           ; <i32>:3 [#uses=1]
         mul i32 %3, ptrtoint (i32* getelementptr (i32* null, i32 1) to 
i32)             ; <i32>:4 [#uses=1]
         call void @BZALLOC( i32 %4 )
         br label %return

return:         ; preds = %entry
         ret void
}

This is generated from this test program:

extern void BZALLOC(int s);
void f(int blockSize100k)
{
     int n = blockSize100k;
     BZALLOC( (n+2) * sizeof(unsigned int) );
}

Besides the optimizer hanging, the strange thing about this is that it 
doesn't hang if the blockSize100k variable is a local rather than a 
parameter or if the n+2 is changed to n+1 (!?!).

Is this code intrinsically incorrect, especially the getelementptr for 
sizeof(), or should I look at the optimizer? It seems to be hanging in 
the createInstructionCombiningPass.

-Rich

Eli Friedman wrote:
> On Sat, May 17, 2008 at 11:34 AM, Richard Pennington <rich at
pennware.com> wrote:
[snip]
> BTW, It's usually better to file a bug for this sort of thing.
I just got a Bugzilla account. I will file a bug next time. Anton did it 
for me this time. (Thanks Anton!)
> 
> The issue is around InstructionCombining:2507:
>   // W*X + Y*Z --> W * (X+Z)  iff W == Y
>   if (I.getType()->isIntOrIntVector()) {
>     Value *W, *X, *Y, *Z;
>     if (match(LHS, m_Mul(m_Value(W), m_Value(X))) &&
>         match(RHS, m_Mul(m_Value(Y), m_Value(Z)))) {
> 
> The issue starts with the lines:
>         add i32 %2, 2           ; <i32>:3 [#uses=1]
>         mul i32 %3, ptrtoint (i32* getelementptr (i32* null, i32 1) to
> i32)             ; <i32>:4 [#uses=1]
> 
> Roughly, the multiplication gets distributed, resulting in something
> like (loaded value) * (ptrtointexpr) + 2 * (ptrtointexpr).  This gets
> matched by the match, which then reverses the transformation.  This,
> of course, gets matched by the code to distribute the multiply,
> resulting in a never-ending cycle.
Thanks for the explanation.
> 
> A side note: I know I've seen suggestions that "ptrtoint (i32*
> getelementptr (i32* null, i32 1) to i32)" is a suitable replacement
> for sizeof, but if it is supposed to be legal, the documentation for
> getelementptr should make that clear. I'm pretty sure the equivalent
> C, "((int*)0)+1", has undefined behavior.
I think that this is not undefined behavior unless the result is 
dereferenced. In particular, I think it would be a common way of 
implementing offsetof().

-Rich

On May 17, 2008, at 1:57 PM, Eli Friedman wrote:
> On Sat, May 17, 2008 at 11:34 AM, Richard Pennington <rich at
pennware.com
> > wrote:
>> If I run the optimizer (opt) on this code snippet with -std-compile- 
>> opts
>> the optimizer hangs.
wow, that's not polite :)
>>
> BTW, It's usually better to file a bug for this sort of thing.
Absolutely.  In any case, this is fixed, patch here:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080512/062585.html

Thanks for letting us know about this!
> A side note: I know I've seen suggestions that "ptrtoint (i32*
> getelementptr (i32* null, i32 1) to i32)" is a suitable replacement
> for sizeof, but if it is supposed to be legal, the documentation for
> getelementptr should make that clear. I'm pretty sure the equivalent
> C, "((int*)0)+1", has undefined behavior.
Is there a great need to do this?  It is useful to have a page that  
explains useful idioms, but there is something to be said for keeping  
langref.html relatively focused on defining semantics, not giving lots  
of examples of use.  If the wording around GEP can be improved, to  
make it more clear that restrictions on C don't apply, then we should  
definitely improve the wording of course.

-Chris