This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Don't adjust align for IBM extended double type
ClosedPublic

Authored by qiucf on Nov 29 2020, 10:13 AM.

Diff Detail

Event Timeline

qiucf created this revision.Nov 29 2020, 10:13 AM
qiucf requested review of this revision.Nov 29 2020, 10:13 AM
clang/lib/CodeGen/TargetInfo.cpp
5055

s/which is/which are/;

and

use getFloatTypeSemantics to reuse logic (including the possible need to handle OpenMP host versus device differences):

} else if (Ty->isRealFloatingType() &&
           getContext().getFloatTypeSemantics(Ty) ==
               &llvm::APFloat::IEEEquad()) {
  // IEEE 128-bit floating numbers are also stored in vector registers, which
  // are quad-word aligned.
  return CharUnits::fromQuantity(16);
}
qiucf updated this revision to Diff 308264.Nov 29 2020, 10:57 PM
qiucf marked an inline comment as done.

use getFloatTypeSemantics

clang/test/CodeGen/ppc64le-varargs-f128.c
27

See comment below.

36–37

Perhaps the argument to the intrinsic should capture a string variable that is used in the later lines to establish the expected relationship of the later lines to this one.

jsji added inline comments.Nov 30 2020, 7:37 AM
clang/lib/CodeGen/TargetInfo.cpp
5055

It would be better to add a testcase for OpenMP host/device format mismatch.

5058

I think it is better to refer to ABI rules instead of referring to vector registers.

Section 2.2.3.3. Optional Save Areas
If extended precision floating-point values in IEEE BINARY 128 QUADRUPLE PRECISION format are supported, map them to a single quadword, quadword aligned. This might result in skipped doublewords in the Parameter Save Area.
qiucf updated this revision to Diff 308864.Dec 1 2020, 9:03 PM
qiucf marked 4 inline comments as done.

Address comments

  • Update comment to refer to ABI
  • Add test case for OpenMP host & target mismatch
jsji accepted this revision as: jsji.Dec 1 2020, 9:05 PM

LGTM. Thanks.

This revision is now accepted and ready to land.Dec 1 2020, 9:05 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2020, 1:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MaskRay added inline comments.
clang/test/CodeGen/ppc64le-varargs-f128.c
8

Generally %clang is only used in test/Driver and %clang_cc1 should be used for tests.

%clang_cc1 has a lit substitution rule to find the builtin include directory where stdarg.h can be found.

qiucf added inline comments.Dec 2 2020, 6:54 PM
clang/test/CodeGen/ppc64le-varargs-f128.c
8

Thanks! I moved it in rG222da77a

MaskRay added inline comments.Dec 2 2020, 7:03 PM
clang/test/CodeGen/ppc64le-varargs-f128.c
8

rG222da77a82d17cbc6b989779e2ba2bb4904bb672 looks more wrong to me: test/Driver tests how the clang driver passes CC1 options to the frontend. The CodeGen should be tested in this directory.

You probably should split %clang to several %clang_cc1 commands and keep the test here.

clang/test/CodeGen/ppc64le-varargs-f128.c
8

I don't think moving the test such that a "driver test" checks code gen output is the direction @MaskRay was pointing to. Changing the RUN line to invoke -cc1 (using %clang_cc1) and leaving the test as a code gen test is what I would have expected.

qiucf added inline comments.Dec 2 2020, 11:16 PM
clang/test/CodeGen/ppc64le-varargs-f128.c
8

Ah, thanks for pointing it out. I simplified the splitted commands from clang -v. https://reviews.llvm.org/D92544 is created for easier view, if that looks fine, I'll commit it :-)