Page MenuHomePhabricator

Treat a narrowing PtrToInt like Trunc when generating asm
ClosedPublic

Authored by mjhostet on Apr 30 2019, 10:43 AM.

Details

Summary

When printing assembly for PtrToInt, AsmPrinter::lowerConstant
incorrectly assumed that if PtrToInt was not converting to an
int with exactly the same number of bits, it must be widening
to a larger int. But this isn't necessarily true; PtrToInt can
also shrink the size, which is useful when you want to produce
a known 32-bit pointer on a 64-bit platform (on x86_64 ELF
this yields a R_X86_64_32 relocation).

The old behavior of falling through to the widening case for a
narrowing PtrToInt yields bogus assembly code like this, which
fails to assemble because the no-op bit and it accidentally
creates is not a valid relocation:

.long   a&-1

The fix is to treat a narrowing PtrToInt exactly the same as
it already treats Trunc: just emit the expression and let
the assembler deal with truncating it in the appropriate way.

Diff Detail

Repository
rL LLVM

Event Timeline

mjhostet created this revision.Apr 30 2019, 10:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2019, 10:43 AM
smith added a subscriber: smith.Apr 30 2019, 10:59 AM
mjhostet edited reviewers, added: lhames, arsenm; removed: grosbach.Apr 30 2019, 11:36 AM
arsenm added inline comments.Apr 30 2019, 12:57 PM
llvm/test/CodeGen/X86/ptrtoint-narrow.ll
2 ↗(On Diff #197374)

Needs to check output. Usually for object emission, the disassembled output is checked

mjhostet marked an inline comment as done.Apr 30 2019, 2:13 PM
mjhostet added inline comments.
llvm/test/CodeGen/X86/ptrtoint-narrow.ll
2 ↗(On Diff #197374)

Sure, I can check output if you want. The reason I didn't is because previously, llc printed the following error and exited with a error status when its internal assembler failed:

fatal error: error in backend: expected relocatable expression

The included test is testing that this no longer happens. I'll check the output too.

mjhostet updated this revision to Diff 197451.Apr 30 2019, 2:52 PM

Check .s output in test, rather than llc exit status creating .o.

mjhostet marked an inline comment as done.Apr 30 2019, 2:53 PM
mjhostet updated this revision to Diff 197461.Apr 30 2019, 3:21 PM

Simplify test

JohnReagan added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
2238 ↗(On Diff #197461)

We made exactly this change in our out-of-tree backend that we use for OpenVMS x86. It was going to be one that we would push when we moved forward, but you beat me to it! Thanks!

hfinkel accepted this revision.May 15 2019, 8:48 AM
hfinkel added a subscriber: hfinkel.

LGTM

This revision is now accepted and ready to land.May 15 2019, 8:48 AM

Thanks @hfinkel! Could you please commit this for me?

ping 3: already approved, just need someone to commit for me. Thanks!

This revision was automatically updated to reflect the committed changes.