[LLVMdev] Patch/Question: calculateFrameObjectOffsets

[Chris Lattner]

On Tue, 8 Jun 2004, Vladimir Prus wrote:

> the calculateFrameObjectOffsets methods in CodeGen/PrologEpilogEmitter.cpp
> does not work for me, since it asserts if stack grows up, and when assert is
> commented out, allocates spill slots in the same location as function
> arguments, which is not right.

Hrm, that's antisocial.  :)  As the assert indicated, we never tested this
on a stack-growing-up target, so I'm not too suprised :)

> I've tried to fix that, and a patches. It merely adds
>    if (StackGrowsDown)
> everywhere, so that the current logic should not be alterted, and the logic
> when stack grows up is correct. I'd say that the patch is not as nice, due to
> those conditionals, but on the other hand the existing logic is a bit tricky,
> and

Ooh, great!  Thanks for the patch (comments below).

> I would not like to change it, especially since I haven't yet learned
> howto run LLVM testsuite.

I would suggest trying to run the LLVM test suite.  It should be pretty
easy.  To try it out, go into:

llvm/test/Programs/MultiSource/Benchmarks/Olden

... and type 'make'.  It should run through all of the programs in that
directory and build them with the CBE, JIT, and LLC targets, run them all
and make sure they work.

If you go to a higher level directory (e.g., test/Programs) it will run
all tests under that directory.

The one really bad thing about the test/Programs system is that there is
no way to set XFAILs currently, so you have to check the nightly tester to
see if something is known to be flagged as an error.

If you have any problems with this, please let us know. :)

> What do you think, is this patch OK or I should try to remove as many of
> conditionals as I can?

I would prefer that you eliminated some of the obvious ones, like this
one:

+    if (StackGrowsDown) {
+        if (FixedOff > Offset) Offset = FixedOff;
+    } else {
+        if (FixedOff > Offset) Offset = FixedOff;
+    }
   }

and fold these two conditionsl together:

-    FFI->setObjectOffset(i, -Offset);        // Set the computed offset
+    if (StackGrowsDown)
+        FFI->setObjectOffset(i, -Offset);        // Set the computed offset
+    else
+        FFI->setObjectOffset(i, Offset);
+
+    if (!StackGrowsDown)
+    {
+        Offset += FFI->getObjectSize(i);
+    }
+
   }

... but otherwise the conditionals and patch as a whole looks great.

If you send and updated patch, I would be happy to apply it for you.  For
patches, please send them to the llvmbugs list:
http://mail.cs.uiuc.edu/mailman/listinfo/llvmbugs

I'd prefer to keep llvmdev a bit higher level (discussing how to do things
and for answering questions) by keeping the patches on a separate list.

Thanks!

-Chris

-- 
http://llvm.cs.uiuc.edu/
http://www.nondot.org/~sabre/Projects/