Page MenuHomePhabricator

Implement _ExtInt ABI for all ABIs in Clang, enable type for ABIs
ClosedPublic

Authored by erichkeane on Apr 29 2020, 12:51 PM.

Details

Summary

This is the result of an audit of all of the ABIs in clang to implement
and enable the type for those targets.

Additionally, this finds an issue with integer-promotion passing for a
few platforms when using _ExtInt of < int, so this also corrects that
resulting in signext/zeroext being on a params of those types in some
platforms.

Diff Detail

Event Timeline

erichkeane created this revision.Apr 29 2020, 12:51 PM
erichkeane updated this revision to Diff 261523.May 1 2020, 1:05 PM
erichkeane retitled this revision from [WIP] _ExtInt calling convention Audit to Implement _ExtInt ABI for all ABIs in Clang, enable type for ABIs.
erichkeane edited the summary of this revision. (Show Details)
erichkeane added a subscriber: cfe-commits.

@rjmccall

Alright, i finished the audit and finished all of the ABIs, as well as wrote tests for each. I added a test for situations that I'd not previously covered as well.

Can you please review when you can/suggest others to do so?

Thanks!

erichkeane updated this revision to Diff 261524.May 1 2020, 1:08 PM
erichkeane edited the summary of this revision. (Show Details)

Remove test for which there is no longer a target where extint isn't supported (thus I cannot spell the condition). Also, clang-formatted the previous revision.

A little cavalier with byval; I gave it a thorough audit, but you might want your own pass.

clang/lib/CodeGen/ABIInfo.h
107

Maybe add a "ForABI" suffix here so that it's clearer that this is a specialized use-case.

clang/lib/CodeGen/TargetInfo.cpp
719

It's very weird for 64 and 128 to be showing up as constants in the default ABI rule.

1540

Won't this treat literally all ExtInt types as either extend or direct?

4946

Does this need to consider the aggregate-as-array logic below?

5022

byval is not legal on return values.

6347

byval is not legal on return types.

6668–6669

Ugh. Please go ahead and rename this to make it clear that it's an NVPTX helper function.

7930

byval is not legal on a return.

8997

Why this change?

10310

Looks like this shouldn't be byval.

erichkeane marked 15 inline comments as done.May 4 2020, 7:46 AM

Thanks for the review @rjmccall . Updated patch coming momentarily. I audited my 'byval' use as well. I still have 1 open, the 'array' comment which I wasn't clear what you meant.

clang/lib/CodeGen/TargetInfo.cpp
719

Good point. I rewrote this in terms of the Context.LongLongTy and Context.Int128Ty.

1540

Yes it does, and I justified doing it that way at one point, but I cannot remember why.

NVPTX seems to pass everything by value in return anyway(including large structures!), so I'm doing that one on purpose.

SparcV9 actually has a 256 bit limit, so I added a test to explicitly check it.

4946

I'm not sure what you mean by this? Are you suggesting I could/should pass this as an array instead of indirectly?

5022

Ah, neat! I didn't realize that. Thanks.

6668–6669

:) Moved to a member function. Also, coerceToIntArrayWithLimit made sense to do so as well, since it was the caller of this.

8997

So that line 9000 will 'extend' 63 bit sizes. getTypeSize rounds to power of 2, getIntWidth doesn't.

As it was unclear, I've instead added this as an explicit case below.

erichkeane updated this revision to Diff 261820.May 4 2020, 7:47 AM
erichkeane marked 5 inline comments as done.
rjmccall added inline comments.Mon, May 4, 12:48 PM
clang/lib/CodeGen/TargetInfo.cpp
4946

My interpretation is that in general you're lowering large a _ExtInt like a struct with a bunch of integer members in it. My understanding is that, for this target, that would make it a homogeneous aggregate eligible for the special treatment given to certain aggregate types below. I don't know what that corresponds to at the code-emission level.

erichkeane updated this revision to Diff 261922.Mon, May 4, 1:47 PM
erichkeane marked 2 inline comments as done.

For PPC64, better emulate structs by passing _ExtInt as an array.

rjmccall added inline comments.Mon, May 4, 7:38 PM
clang/lib/CodeGen/TargetInfo.cpp
4946

This seems to work, although it's unfortunate that it duplicates some of the existing behavior. Do we also need to modify isHomogeneousAggregate to consider ExtInts? And if we do, does that avoid any of the duplicate logic here?

6668–6669

Thanks.

jdoerfert resigned from this revision.Mon, May 4, 8:22 PM
erichkeane marked an inline comment as done.Tue, May 5, 6:07 AM
erichkeane added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
4946

I think doing that would result in altering a number of other targets as well (thats not a PPC specific function I think?).

While there IS a little duplication, it is just the 5 lines that calculates the array and coerces it. Otherwise the logic is just slightly different enough I don't think it would save much.

That said, perhaps there is value in extracting those 5 lines or so into a function. I'll give that a shot.

erichkeane updated this revision to Diff 262093.Tue, May 5, 6:19 AM

Extracted out a function for PPC64's coerce to array.

rjmccall added inline comments.Tue, May 5, 10:13 AM
clang/lib/CodeGen/TargetInfo.cpp
4946

Yeah, the function isn't PPC-specific. The question is what the right rules are for homogeneous aggregates when aggregates contain ExtInt types. For PPC64 ELFv2, that... oh right, it only applies to floating-point and vector types. So I'm sorry, I led you on a false path here; we should not be trying to apply this logic to ExtInt types at all.

erichkeane marked an inline comment as done.Tue, May 5, 10:48 AM
erichkeane added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
4946

So I should switch it back to direct/indirect like earlier? That I can do.

Revert PPC64 changes back to direct/indirect.

rjmccall added inline comments.Tue, May 5, 1:45 PM
clang/lib/CodeGen/TargetInfo.cpp
719

Maybe:

if (EIT->getNumBits() > Context.getTypeSize(Context.getTargetInfo().hasInt128Type() ? Context.Int128Ty : Context.LongLongTy)))
742

Same rule needed here.

erichkeane updated this revision to Diff 262274.Tue, May 5, 5:49 PM
erichkeane marked 5 inline comments as done.
rjmccall accepted this revision.Tue, May 5, 10:44 PM

Okay, LGTM.

This revision is now accepted and ready to land.Tue, May 5, 10:44 PM