[LLVMbugs] [PATCH] calculateFrameObjectOffsets

[Chris Lattner]

On Wed, 9 Jun 2004, Vladimir Prus wrote:

> Chris Lattner wrote:
> > ... 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.
>
> Ok, running now. What is relation between Makefile-based tests and QMTest? At
> least for me QMTest is more convenient? And also, what version of QMTest did
> you use?

The QMTest tests are usually for regressions: checking that something
implemented does not break in the future.  As such, they are usually .ll
files just big enough to trigger the feature.

test/Programs are whole programs that are expected to compile and work
with all of the optimizers and code generators.  They are much more
holistic tests and are more likely to be useful for you right now.  As you
find and fix (nontrivial) bugs, you should consider adding regression
tests to the test/CodeGen/XXX directory for your target.  You can take a
look at the X86 tests to see how they are done.

> Ok, I've made a second pass through the function, tweaking things and adding
> comments and finally I like the result.
>
> The patch also makes X86 and PowerPC backends to pass -4 as local area offset
> instead of 4 (not matter that PowerPC is not finished). The
> calculateFrameObjectOffsets is accordingly modified.

Here are a couple of comments:


@@ -203,26 +202,51 @@
-  // Check to see if there are any fixed sized objects that are preallocated in
-  // the local area.  We currently don't support filling in holes in between
-  // fixed sized objects, so we just skip to the end of the last fixed sized
+  if (StackGrowsDown)
+      Offset = -Offset;
+  assert(Offset >= 0);

Please put a string into the assertion (assert(Offset >= 0 && "foo")) so
that we get a potentially useful indication of what when wrong if this
ever triggers.

   for (int i = FFI->getObjectIndexBegin(); i != 0; ++i) {
-    int FixedOff = -FFI->getObjectOffset(i);
-    if (FixedOff > Offset) Offset = FixedOff;
+    int FixedOff;
+    if (StackGrowsDown) {
+        // The maximum distance from the stack pointer is the lower address of
+        // the object -- which is exactly its offset.
+        FixedOff = FFI->getObjectOffset(i);

Okay, it looks like you are missing a - in the StackGrowsDown case.
Before FixedOff was -FFI->getObjectOffset(i), now it's not.  Is this
intentional?

+    } else {
+        // The maximim distance from the stach pointer is the upper address of
+        // the object.

2 spello's.

Otherwise it looks good.  Resend and we'd be happy to apply it for you,
thanks!!

-Chris

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