[PATCH] D16788: PS4 ABI Round 2. Actual PS4 code.
John McCall via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 1 22:40:22 PST 2016
rjmccall added inline comments.
================
Comment at: include/clang/Basic/TargetCXXABI.h:118
@@ -115,1 +117,3 @@
+ /// in LLVM 3.2.
+ PS4
};
----------------
I'm not sure why you added a new C++ ABI kind here. The bug fix you're opting out of is not at all specific to C++, and there are more straightforward ways to check the target than checking the C++ ABI kind.
I mean, I have no doubt that eventually there will be some significant C++ ABI bug fix that you don't want to pick up, so I'm not opposed to adding a new C++ ABI kind. It just seems inappropriate to do that in this patch.
================
Comment at: lib/AST/RecordLayoutBuilder.cpp:1598-1599
@@ -1597,1 +1597,4 @@
+ // PS4 remains compatible to pre r257462 behavior.
+ bool isPS4ABI = (Context.getTargetInfo().getCXXABI().getKind() == TargetCXXABI::PS4);
+
----------------
rsmith wrote:
> Please say what that behavior was here. "The PS4 ABI ignores explicit alignment attributes on bitfields." or similar.
Also, this should be added as a bit on TargetInfo instead of making it a target-specific check in random code. This also makes it easier to make the code self-documenting, because instead of:
} else if (ExplicitFieldAlign && !isPS4ABI) {
you'd have
} else if (ExplicitFieldAlign && Context.getTargetInfo().useExplicitBitFieldAlignment()) {
You can then document the fact that your ABI is explicitly opting out of this bug fix in a comment on the line in Targets.cpp where you set this field to false.
http://reviews.llvm.org/D16788
More information about the cfe-commits
mailing list