This is an archive of the discontinued LLVM Phabricator instance.

[x86 fast-isel] Materialize allocas with the correct-sized lea for ILP32
ClosedPublic

Authored by dschuff on Nov 5 2014, 11:01 AM.

Details

Summary

X86FastISel::fastMaterializeAlloca was incorrectly conditioning its
opcode selection on subtarget bitness rather than pointer size.

Diff Detail

Repository
rL LLVM

Event Timeline

dschuff updated this revision to Diff 15817.Nov 5 2014, 11:01 AM
dschuff retitled this revision from to [x86 fast-isel] Materialize allocas with the correct-sized lea for ILP32.
dschuff updated this object.
dschuff edited the test plan for this revision. (Show Details)
dschuff added a reviewer: jvoung.
dschuff added a subscriber: Unknown Object (MLST).
jvoung added inline comments.Nov 5 2014, 11:16 AM
test/CodeGen/X86/x32-alloca.ll
2 ↗(On Diff #15817)

Add "-fast-isel-abort" to make sure it's actually using fast-isel for this codegen?

A bunch of the other fast-isel tests have a "fast-isel-" prefix for the filename. Perhaps it would be good to stick with that.

I couldn't find any existing test for this for non ILP32 x86-64 code, so might as well test the non-ILP32 x86-64 code path too. Maybe the test can be merged into fast-isel-x86-64.ll (but only have a check-prefix of this case for x32 for now)?

6 ↗(On Diff #15817)

could probably just make bar a "declare" and not have to codegen it

rnk accepted this revision.Nov 5 2014, 11:17 AM
rnk added a reviewer: rnk.
rnk added a subscriber: rnk.

lgtm

I suspect we'll see more issues like this.

test/CodeGen/X86/x32-alloca.ll
17 ↗(On Diff #15817)

please fix

This revision is now accepted and ready to land.Nov 5 2014, 11:17 AM
dschuff updated this revision to Diff 15819.Nov 5 2014, 11:33 AM
dschuff edited edge metadata.
  • Review comments
test/CodeGen/X86/x32-alloca.ll
2 ↗(On Diff #15817)

Added -fast-isel-abort and renamed; unfortunately fast-isel-x86_64.ll can't be codegen'd for x32/nacl currently (it throws an assert). Fixing that would be beyond the scope of this change but I added the same test case to that file for LP64.

6 ↗(On Diff #15817)

Done.

17 ↗(On Diff #15817)

Done.

dschuff updated this revision to Diff 15820.Nov 5 2014, 11:34 AM
  • really fix newline
dschuff closed this revision.Nov 5 2014, 11:38 AM
dschuff updated this revision to Diff 15821.

Closed by commit rL221386 (authored by @dschuff).