Page MenuHomePhabricator

[X86] Refactor GetSSETypeAtOffset to fix pr51813
ClosedPublic

Authored by pengfei on Sep 10 2021, 8:05 AM.

Details

Summary

D105263 adds support for _Float16 type. It introduced a bug (pr51813) that generates a <4 x half> type instead the default double when passing blank structure by SSE registers.

Although I doubt it may expose a bug somewhere other than D105263, it's good to avoid return half type when no half type in arguments.

Diff Detail

Event Timeline

pengfei requested review of this revision.Sep 10 2021, 8:05 AM
pengfei created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2021, 8:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
pengfei updated this revision to Diff 372118.Sep 12 2021, 6:19 AM

Fix lit fails.

pengfei retitled this revision from [X86][WIP] Refactor GetSSETypeAtOffset to [X86] Refactor GetSSETypeAtOffset to fix pr51813.Sep 12 2021, 6:33 AM
pengfei edited the summary of this revision. (Show Details)
pengfei updated this revision to Diff 372119.Sep 12 2021, 6:38 AM

Fix typo.

pengfei updated this revision to Diff 372120.Sep 12 2021, 6:43 AM

Change to use getFPTypeAtOffset.

pengfei updated this revision to Diff 372122.Sep 12 2021, 6:46 AM

Fix format.

pengfei updated this revision to Diff 372123.Sep 12 2021, 7:23 AM

Ues getTypeAllocSize.

LuoYuanke added inline comments.Sep 12 2021, 8:50 PM
clang/lib/CodeGen/TargetInfo.cpp
3446

Would you add comments on each case like previous code?

3484

Is this the major change?

pengfei updated this revision to Diff 372254.Sep 13 2021, 7:56 AM
pengfei marked an inline comment as done.

Add more comments.

pengfei added inline comments.Sep 13 2021, 8:06 AM
clang/lib/CodeGen/TargetInfo.cpp
3484

No, this logic is not changed. When T1 is nullptr, it means IRType is simply a half or float. So we return T0 that equals to the code here.

LuoYuanke added inline comments.Sep 15 2021, 1:32 AM
clang/lib/CodeGen/TargetInfo.cpp
3451

Not quite understanding why "+4". Would you comments on it?

LuoYuanke added inline comments.Sep 15 2021, 1:36 AM
clang/test/CodeGen/X86/avx512fp16-abi.c
153

Add a test case for "{ struct {}; half; struct {}; half;}?

pengfei updated this revision to Diff 372664.Sep 15 2021, 2:16 AM
pengfei marked 2 inline comments as done.

Address Yuanke's comments.

LuoYuanke accepted this revision.Sep 15 2021, 7:29 AM

LGTM, but pls wait 1 or 2 days for the comments from others.

This revision is now accepted and ready to land.Sep 15 2021, 7:29 AM

This patch seem to have broken GCC-C-execute-pr44575 from the llvm-test-suite. See http://meinersbur.de:8011/#/builders/76/builds/761 (this builder compiles with Polly, but it also crashes without Polly)

This patch seem to have broken GCC-C-execute-pr44575 from the llvm-test-suite. See http://meinersbur.de:8011/#/builders/76/builds/761 (this builder compiles with Polly, but it also crashes without Polly)

Thanks @Meinersbur for reporting this. Do you have a small reproducer or the crash log? I didn't find any detail about the crash on the bot.

This patch seem to have broken GCC-C-execute-pr44575 from the llvm-test-suite. See http://meinersbur.de:8011/#/builders/76/builds/761 (this builder compiles with Polly, but it also crashes without Polly)

@Meinersbur, sorry for the late response. I just managed to reproduce the failure. I create D110037 to try to fix this problem. The test passed locally.
By the way, does this bot send notification to authors when it fails? I didn't receive this fail, so I'm not aware of it at the first time.

By the way, does this bot send notification to authors when it fails? I didn't receive this fail, so I'm not aware of it at the first time.

This is my personal staging buildbot server to park my worker until my zorg patches are greenlit. Hence, I disabled sending any email notification. However, my other one that is connected to lab.llvm.org has failed as well and should have sent an email: https://lab.llvm.org/buildbot/#/builders/102/builds/2722. Unfortunately it is slow and packing to many commits together, which I am trying to improve: D110048

Independent of that, it seems no other builder running the test-suite on x86_64 (there are armv7, s390x and ppc64le ones)

However, my other one that is connected to lab.llvm.org has failed as well and should have sent an email: https://lab.llvm.org/buildbot/#/builders/102/builds/2722. Unfortunately it is slow and packing to many commits together, which I am trying to improve: D110048

I didn't receive it either. I once suspected my mailbox but haven't had any fortune for now. :(

Independent of that, it seems no other builder running the test-suite on x86_64 (there are armv7, s390x and ppc64le ones)

Thanks for your information.

However, my other one that is connected to lab.llvm.org has failed as well and should have sent an email: https://lab.llvm.org/buildbot/#/builders/102/builds/2722. Unfortunately it is slow and packing to many commits together, which I am trying to improve: D110048

I didn't receive it either. I once suspected my mailbox but haven't had any fortune for now. :(

Might have because that somehow the master thinks the job before the one that is breaking is still building, ie. cannot identify whether the failure is new.

However, my other one that is connected to lab.llvm.org has failed as well and should have sent an email: https://lab.llvm.org/buildbot/#/builders/102/builds/2722. Unfortunately it is slow and packing to many commits together, which I am trying to improve: D110048

I didn't receive it either. I once suspected my mailbox but haven't had any fortune for now. :(

Might have because that somehow the master thinks the job before the one that is breaking is still building, ie. cannot identify whether the failure is new.

Got it, thanks Michael.

dyung added a subscriber: dyung.Sep 29 2021, 8:15 AM

Hi, our internal testing started to hit an assertion failure in one of our tests after this commit. I have put the details in PR52011, can you please take a look?

Hi, our internal testing started to hit an assertion failure in one of our tests after this commit. I have put the details in PR52011, can you please take a look?

Sure. Thanks for reporting it.