This is an archive of the discontinued LLVM Phabricator instance.

ARM: centralise SizeType, PtrDiffType, and IntPtrType
ClosedPublic

Authored by compnerd on Oct 25 2017, 10:56 PM.

Details

Summary

Centralise the definitions of these compiler vended types to aid
inspection to ensure that they are defined similarly. The one case that
stands out is the Darwin case where the types do not match up. This
fixes the API conformance for APCS-GNU as well.

Diff Detail

Repository
rL LLVM

Event Timeline

compnerd created this revision.Oct 25 2017, 10:56 PM
fhahn added a subscriber: fhahn.Oct 26 2017, 2:03 AM

Could you upload a diff with more context, as suggested in http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface? That would make it easier to review the patch.

fhahn added a reviewer: fhahn.Oct 26 2017, 2:03 AM

I think you missed a couple other places which still set SizeType and PtrDiffType?

compnerd updated this revision to Diff 120534.Oct 26 2017, 7:40 PM

Catch a couple of missed instances, add more context

compnerd added inline comments.Oct 26 2017, 10:07 PM
lib/Basic/Targets/ARM.cpp
231

I changed this to:

if (Triple.isOSDarwin() && !Triple.isWatchABI())
  PtrDiffType = SignedInt;

Are you sure the change to APCS is right? I mean, it looks like it's right if I compare to gcc with -mabi=gnu-apcs, but I'm not sure what, exactly, we're trying to be compatible with, so I'd prefer not to touch it, especially not in a patch with a bunch of changes which are supposed to be NFC.

Well, without matching that ABI, I think that centralizing the logic isn't any cleaner, since we determine the ABI later. With this, we also match the ABI as GNU defines it, and we can move the logic to the same location. intptr_t on Darwin && !WatchOS has the one incompatibility.

efriedma accepted this revision.Oct 27 2017, 4:00 PM

I guess... fine.

LGTM, assuming we have test coverage for all the different cases.

This revision is now accepted and ready to land.Oct 27 2017, 4:00 PM

Yeah, we have coverage for the various environments in test/Preprocessor/init.c. The one case that I didn't find was the APCS-GNU case, which I added a test for additional coverage.

compnerd closed this revision.Oct 27 2017, 4:04 PM

SVN r316810