This is an archive of the discontinued LLVM Phabricator instance.

[X86] Passing union type through register.
ClosedPublic

Authored by LiuChen3 on Apr 23 2020, 3:51 AM.

Details

Summary

For example:

union M256 {
  double d;
  __m256 m;
};

extern void foo1(union M256 A);

union M256 m1;

void test() {
  foo1(m1);
}

clang will pass it through stack because because the judgement: Size != getContext().getTypeSize(i->getType()) will return true.
This is not correct for union type.

Also, I think there are some problems on comments. I did some change.

Diff Detail

Event Timeline

LiuChen3 created this revision.Apr 23 2020, 3:51 AM
RKSimon edited reviewers, added: rsmith; removed: RKSimon.Apr 23 2020, 4:07 AM
RKSimon added a subscriber: RKSimon.
craig.topper added inline comments.Apr 23 2020, 6:29 PM
clang/lib/CodeGen/TargetInfo.cpp
3064

Is there not a isUnion() or isUnionType()?

LiuChen3 updated this revision to Diff 259782.Apr 23 2020, 7:00 PM

Address the comment.

LiuChen3 marked an inline comment as done.Apr 23 2020, 7:01 PM
LiuChen3 added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
3064

Yes. Thanks for the advise.

rjmccall added inline comments.Apr 24 2020, 10:42 AM
clang/lib/CodeGen/TargetInfo.cpp
3089

This change seems correct, but I would be a lot happier if we just extended the Lo/Hi logic properly. The current code is making a lot of questionable assumptions about struct layout.

LiuChen3 marked an inline comment as done.Apr 27 2020, 2:00 AM
LiuChen3 added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
3089

I will work on it, later.

LiuChen3 marked an inline comment as done and an inline comment as not done.Jun 17 2020, 10:54 PM
LiuChen3 added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
3089

Hi, @rjmccall.
I have checked the logic here. I think the we extended the Lo/Hi logic properly if we exclude union type. For union type, I think we should assume that all elements are the same size as union itself. So the class of any size of union type can be determined like the union which size no bigger than 128. What's your concern here, maybe you can give me an example?

LiuChen3 added inline comments.Sep 1 2020, 2:02 AM
clang/lib/CodeGen/TargetInfo.cpp
3089

I think at least the method of determining the number of elements by judging whether the element size and the aggregate size are equal is wrong for union type. I am afraid that extending the current Lo/Hi logic will cause many failures.

Hi, @majnemer, @bruno. I saw that both of you have modified this logic. What's your opinion here?

bruno added a comment.Sep 22 2020, 1:38 PM

Hi, @majnemer, @bruno. I saw that both of you have modified this logic. What's your opinion here?

It has been a long time to remember details, but I share @rjmccall concerns. @craig.topper wdyt?

clang/test/CodeGen/passing-union.c
1 ↗(On Diff #259782)

You probably don't need -fblocks here.

5 ↗(On Diff #259782)

Instead of this include can you just define __m256 and __m512?

LiuChen3 updated this revision to Diff 293628.Sep 22 2020, 7:51 PM

Address comments

LiuChen3 marked 2 inline comments as done.Sep 22 2020, 7:59 PM

It has been a long time to remember details, but I share @rjmccall concerns.

Thanks, @bruno. Modifying the existing logic is risky. If we have to do that, I think a new patch will be better. The purpose of this patch is to solve an ABI bug.

LiuChen3 updated this revision to Diff 293629.Sep 22 2020, 8:03 PM

Include the context.

Thanks, @bruno. Modifying the existing logic is risky. If we have to do that, I think a new patch will be better. The purpose of this patch is to solve an ABI bug.

Ok, I think it's reasonable that you get to fix this bug without having to redesign the Lo/Hi logic here. Two more things:

  • Can you rewrite part of the comment and add a more explicit FIXME for the Lo/Hi stuff?
  • Place the test in clang/test/CodeGen/X86 and rename it to avx-union.c
LiuChen3 updated this revision to Diff 294608.Sep 27 2020, 11:09 PM

Address comments.

bruno accepted this revision.Oct 8 2020, 7:18 PM

LGTM

This revision is now accepted and ready to land.Oct 8 2020, 7:18 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2020, 8:25 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

LGTM

Thanks for your review.

dyung added a subscriber: dyung.Oct 13 2020, 1:45 PM

Hi, the test you added seems to pass both before and after your change, is this intended?

dyung added inline comments.Oct 13 2020, 2:31 PM
clang/test/CodeGen/X86/avx-union.c
2

These RUN lines have 2 issues, you are using "||" instead of "|" and each check prefix must have its own argument, so you need "--check-prefix=CHECK --check-prefix=AVX".

22

When I fix the run lines above, I'm not getting "dso_local". Might want to double check that.

Hi, the test you added seems to pass both before and after your change, is this intended?

Oh, yes. I made mistake on my RUN line. So these tests are not really checked. Thanks for your help!
I will make another patch to fix this testcase.

clang/test/CodeGen/X86/avx-union.c
22

There actually have no 'dso_local'.