Page MenuHomePhabricator

Add support for lowering 32-bit/64-bit pointers
ClosedPublic

Authored by akhuang on Oct 30 2019, 1:31 PM.

Details

Summary

This follows a previous patch that changes the X86 datalayout to represent
mixed size pointers (32-bit sext, 32-bit zext, and 64-bit) with address spaces
(https://reviews.llvm.org/D64931)

This patch implements the address space cast lowering to the corresponding
sign extension, zero extension, or truncate instructions.

Related to https://bugs.llvm.org/show_bug.cgi?id=42359

Diff Detail

Event Timeline

akhuang created this revision.Oct 30 2019, 1:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2019, 1:31 PM
arsenm added a subscriber: arsenm.Oct 30 2019, 2:14 PM
arsenm added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
2451–2452

These look redundant with the size check to me

llvm/test/CodeGen/X86/mixed-ptr-sizes.ll
37–38

Don't need this

109

Should add some tests with the recognized address spaces and an arbitrary number

111

All the metadata should be removed

akhuang marked 3 inline comments as done.Nov 4 2019, 11:36 AM
akhuang added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
2451–2452

Good point - I guess removing the address space check would allow casting from arbitrary address spaces to ptr32/ptr64, which makes sense.

akhuang updated this revision to Diff 227753.Nov 4 2019, 11:36 AM

-Edit logic for when to do address space casts to allow for casting
between ptr32/ptr64 and arbitrary other address spaces; add tests
-Remove unnecessary stuff from test

RKSimon added inline comments.Tue, Nov 12, 6:50 AM
llvm/lib/Target/X86/X86.h
168

use regular X86 namespace and name the enum?

namespace X86 {
enum class AddressSpace : unsigned {
akhuang marked an inline comment as done.Wed, Nov 13, 11:24 AM
akhuang added inline comments.
llvm/lib/Target/X86/X86.h
168

I tried using an enum class but since address spaces are ints it is a bit inconvenient to convert them to enum types. is it better to not have different namespaces?

rnk accepted this revision.Tue, Dec 3, 2:43 PM

lgtm

llvm/lib/Target/X86/X86.h
168

Most code treats address spaces as unsigned integers, I'd leave it as a regular enum for now.

llvm/lib/Target/X86/X86ISelLowering.cpp
27780

This value will appear to be unused in a build without assertions, which will create warnings. The simplest fix would be to skip the local and move this call into the assert.

28778

ditto

This revision is now accepted and ready to land.Tue, Dec 3, 2:43 PM
akhuang updated this revision to Diff 231999.Tue, Dec 3, 3:02 PM

Address comment

This revision was automatically updated to reflect the committed changes.