This is an archive of the discontinued LLVM Phabricator instance.

Extend or truncate __ptr32/__ptr64 pointers when dereferenced.
ClosedPublic

Authored by akhuang on Jun 9 2020, 4:47 PM.

Details

Summary

A while ago I implemented the functionality to lower Microsoft ptr32
and
ptr64 pointers, which are basically stored as 32-bit and 64-bit
pointers and are extended/truncated to the appropriate pointer size when
dereferenced. Apparently I never implemented this extend/truncate when
dereferencing part, so that is what this patch attempts to do.

For 32-bit pointers in 64-bit builds I put the sign/zero extend in
matchAddress in X86ISelDAGToDAG, but loading from 64-bit pointers in
32-bit builds fails in the legalizer before that point, which is why
this code is in two different places. Not sure if this is a good way of doing
this.

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

Diff Detail

Event Timeline

akhuang created this revision.Jun 9 2020, 4:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2020, 4:47 PM
hans added a comment.Jun 10 2020, 5:17 AM

which is why this code is in two different places

Could both cases be handled in the place that runs before the legalizer?

Hopefully Craig can take a look too, he's the expert here.

llvm/test/CodeGen/X86/mixed-ptr-sizes.ll
205

This just shows the sign extension. I would add a check line for the actual load too, to make sure that's correct and also to make the test easier to understand. Same for the tests below.

Thanks for tackling this. Again, comparing this to what I have on my proprietary system with dual sized pointers, I have some observations (and some confusions on my part I'm sure)

For 32-bit targets, can you even talk about ptr64? My old 32-bit platforms don't allow it at all. However, your custom lowering for 32-bit targets seems to imply that you want to support ptr64 containers on 32-bit platforms by always truncating them. Or is the model that a 32-bit target would simply not use those two address spaces and therefore not generate the size/zero extension?

I'm not sure what "custom" means but you only set that for 32-bit targets. Does that mean the zero/sign-extension on the load doesn't occur for 64-bit targets? You still want to zero/sign-extend a ptr32 in a 64-bit target, yes? From what I think I read, the truncation of ptr32 for the store will only be for on 32-bit targets.

My memory was that you'd be able to mix-and-match ptr32 and ptr64 on 64-bit targets since you still need ptr32 to deal with legacy OS interfaces and such.

hans added a comment.Jun 10 2020, 7:05 AM

Thanks for tackling this. Again, comparing this to what I have on my proprietary system with dual sized pointers, I have some observations (and some confusions on my part I'm sure)

For 32-bit targets, can you even talk about ptr64? My old 32-bit platforms don't allow it at all. However, your custom lowering for 32-bit targets seems to imply that you want to support ptr64 containers on 32-bit platforms by always truncating them. Or is the model that a 32-bit target would simply not use those two address spaces and therefore not generate the size/zero extension?

It's following MSVC, whose behaviour can be observed here: https://godbolt.org/z/uNwjMW

I'm worried that load/store aren't the only things that can use these pointers. Or that we will have transformed the load/store into a target specific load/store before type legalizaton kicks in. I'm not an expert on address spaces, but it feels to me like we should have an address space cast in IR before the pointers are used by load/store. And that the address space cast is what should be turned into trunc or extend. @arsenm what do you think?

I'm worried that load/store aren't the only things that can use these pointers. Or that we will have transformed the load/store into a target specific load/store before type legalizaton kicks in. I'm not an expert on address spaces, but it feels to me like we should have an address space cast in IR before the pointers are used by load/store. And that the address space cast is what should be turned into trunc or extend. @arsenm what do you think?

I don't think the addressing mode matching should be responsible for inserting this (it's also suspicious that this would need to look at the memory address space)

Part of me thinks we should just rewrite the loads/stores as early as possible using combineLoad/combineStore to use a ADDRSPACECAST. Then let the existing legalization of ADDRSPACECAST take care of it. And not change the setOperationAction for loads/store. I get a little nervous that something somewhere will care if an i32 load is Custom instead of Legal.

akhuang updated this revision to Diff 270244.Jun 11 2020, 3:02 PM
akhuang marked an inline comment as done.

Put addr space casting in combineLoad/combineStore.

Thanks for the comments! I moved the addr space stuff to combineLoad/combineStore, which ends up being a lot simpler.

Can you rebase this patch? I added a 32-bit version of mixed-ptr-sizes.ll as mixed-ptr-sizes-i686.ll the other day. And the X86ISelDAGToDAG.cpp changes should already be done in trunk as well.

akhuang updated this revision to Diff 270779.Jun 15 2020, 9:35 AM

Rebase and add more check lines to the tests.

@akhuang If you rebase the tests can now be regenerated with the update script

@RKSimon oh, cool. thanks!

akhuang updated this revision to Diff 271504.Jun 17 2020, 3:33 PM

Rebase tests.

please can you check the patch and rebase again? It looks like some of the code is now in the wrong function?

akhuang updated this revision to Diff 272527.Jun 22 2020, 12:58 PM

Rebase, move code to the correct place

sorry about that! It now should be the intended change.

This revision is now accepted and ready to land.Jun 26 2020, 10:07 AM
This revision was automatically updated to reflect the committed changes.