Page MenuHomePhabricator

Allow EmitVAArg() to promote types and use this to fix some N32/N64 vararg issues for Mips.
ClosedPublic

Authored by dsanders on Nov 13 2014, 6:27 AM.

Diff Detail

Event Timeline

dsanders updated this revision to Diff 16155.Nov 13 2014, 6:27 AM
dsanders retitled this revision from to Allow EmitVAArg() to promote types and use this to fix some N32/N64 vararg issues for Mips..
dsanders updated this object.
dsanders edited the test plan for this revision. (Show Details)
dsanders added reviewers: atanasyan, theraven.
dsanders added a subscriber: Unknown Object (MLST).
dsanders updated this revision to Diff 16325.Nov 18 2014, 5:46 AM

Bring this patch into line with the overall way David solved the issue with int
not being a promotable integer. That is, promote the Clang type rather than
promote the LLVM IR type.

There's a still a couple differences compared to the original patches this is
based on:

  • intptr_t/uintptr_t is 32-bit for N32 so we explicitly ask for a 64-bit integer type.
  • We only promote if the integer type is smaller than the one we promote to. This prevents accidental demotion of 128-bit ints.
theraven accepted this revision.Nov 18 2014, 7:06 AM
theraven edited edge metadata.

That looks a lot cleaner than my hacks!

lib/CodeGen/CGExprScalar.cpp
3318

I think the real fix here is to move the CreateLoad into each of the EmitVAArg() call, but this is a simpler change.

This revision is now accepted and ready to land.Nov 18 2014, 7:06 AM
atanasyan accepted this revision.Nov 18 2014, 7:18 AM
atanasyan edited edge metadata.
dsanders closed this revision.Nov 19 2014, 2:01 AM

Thanks, committed in r222339.

lib/CodeGen/CGExprScalar.cpp
3318

I agree. The load for the argument is also target specific so it belongs with the target specific code.

It shouldn't be difficult to move it but the obvious patch will affect all targets. If I manage to find some spare time, I'll take a look at making that change.

By the way, I'll have to try some small structures (<32-bit for O32, <64-bit for N32/N64) in varargs soon. It's occurred to me that they probably don't work for any big-endian ABI assuming they are defined the same way in varargs as they are for fixed args. If that turns out to be correct then moving the load into EmitVAArg() will be a prerequisite of fixing it since I'll need to shift the result of the load to the right.