This is an archive of the discontinued LLVM Phabricator instance.

Implement inalloca codegen for x86 with the new inalloca design
ClosedPublic

Authored by rnk on Jan 27 2014, 4:23 PM.

Details

Summary

Calls with inalloca are lowered by skipping all stores for arguments
passed in memory and the initial stack adjustment to allocate argument
memory.

Now the frontend is responsible for the memory layout, and the backend
doesn't have to do any work. As a result these changes are pretty
minimal.

Diff Detail

Event Timeline

rnk updated this revision to Unknown Object (????).Jan 29 2014, 1:56 PM
  • Minor tweaks
echristo added inline comments.Jan 30 2014, 5:49 PM
include/llvm/Support/CallSite.h
271 ↗(On Diff #6753)

This change seems weird. Even if it's documented to be the last one I'm not sure why...

lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
78 ↗(On Diff #6753)

Eh? I think I see it but it could use a comment.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5386 ↗(On Diff #6753)

Should probably document this assertion somewhere?

7096 ↗(On Diff #6753)

This looks weird.

7323 ↗(On Diff #6753)

This looks weird. What's up?

lib/Target/X86/X86FastISel.cpp
2490 ↗(On Diff #6753)

Comment?

lib/Target/X86/X86FrameLowering.cpp
1495 ↗(On Diff #6753)

Might want to explain when we would expect it to be negative.

1529 ↗(On Diff #6753)

Ditto.

lib/Target/X86/X86ISelLowering.cpp
2933 ↗(On Diff #6753)

Rename this in a separate patch?

rnk added a comment.Jan 30 2014, 6:42 PM

I'll split some stuff and reupload.

include/llvm/Support/CallSite.h
271 ↗(On Diff #6753)

It has to be last because it holds all the arguments passed in memory, and memory arguments are always the last parameters in all supported calling conventions.

lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
78 ↗(On Diff #6753)

sure

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5386 ↗(On Diff #6753)

Added some docs. I'm not really sure it's worth it though. This feature was added in r86876 by Kenneth Uildricks (remember him?) and never documented, so far as I know.

7096 ↗(On Diff #6753)

I'll try to expand this comment here.

7323 ↗(On Diff #6753)

ditto

lib/Target/X86/X86FastISel.cpp
2490 ↗(On Diff #6753)

sure

lib/Target/X86/X86FrameLowering.cpp
1495 ↗(On Diff #6753)

I think this only happened in an intermediate state when I had bugs, because CalleeAmt was greater than Amount. I'm going to commit this separately as an assertion that Amount is positive. addImm takes a signed int64_t, so the signed type seems correct here.

lib/Target/X86/X86ISelLowering.cpp
2933 ↗(On Diff #6753)

Sure.

rnk updated this revision to Unknown Object (????).Jan 31 2014, 11:18 AM
  • add more comments and docs
  • revert frame lowering changes
  • rebased
echristo added inline comments.Jan 31 2014, 3:17 PM
include/llvm/Support/CallSite.h
271 ↗(On Diff #6803)

This could/should probably be verified by the IR verifier if it isn't already.

lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
80 ↗(On Diff #6803)

This is a general change in behavior for non-inalloca code and could probably use a test case.

lib/IR/Mangler.cpp
68 ↗(On Diff #6803)

Period on the end of the sentence (yes, I know you didn't write it).

lib/Target/X86/X86FastISel.cpp
2490 ↗(On Diff #6803)

This also seems like it should be in a separate check-in similar to the one above that checks basically for the same thing.

rnk added inline comments.Jan 31 2014, 3:37 PM
include/llvm/Support/CallSite.h
271 ↗(On Diff #6803)

Yep, it is.

lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
80 ↗(On Diff #6803)

Sure, but it will require inalloca, because that's the only way to get a non-static alloca here.

lib/IR/Mangler.cpp
68 ↗(On Diff #6803)

sure

lib/Target/X86/X86FastISel.cpp
2490 ↗(On Diff #6803)

sure

echristo accepted this revision.Jan 31 2014, 3:54 PM
rnk closed this revision.Jan 31 2014, 3:57 PM

Closed by commit rL200596 (authored by @rnk).