[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

Mon Oct 10 15:45:52 PDT 2022

rnk added a comment.

I think in the end, perhaps it is not worth disturbing this code.

================
Comment at: clang/lib/Basic/Targets/OSTargets.h:169
+    if (T.getOS() == llvm::Triple::WatchOS ||
+        this->getCXXABI().getKind() == TargetCXXABI::AppleARM64)
+      return TargetInfo::UseTailPaddingUnlessPOD11;
----------------
dblaikie wrote:
> dblaikie wrote:
> > rnk wrote:
> > > I think it would be equivalent to check `T.getArch() == llvm::Triple::AArch64`, that's probably what set the TargetCXXABI.
> > Hmm, seems one of the tests failed because apparently CXXABI can be chosen separately from target. `clang/test/CodeGenCXX/armv7k.cpp` for instance runs with `-triple=arm64_32-apple-ios -emit-llvm -target-abi darwinpcs`
> > 
> > Which seems to have an UnknownOS on the triple, but a WatchOS on the TargetCXXABI... (I don't really understand/haven't looked into how all these things relate to the command line arguments there - none of it mentions WatchOS, but maybe `darwinpcs` is the name of watchOS?
> > 
> > Updating this function to inspect the CXXABI for WatchOS, AppleARM64, and iOS (which I'd missed from here previously) here makes the tests pass... 
> > 
> > Maybe that suggests we should move all this (& the D119051) to TargetCXXABI?
> I guess if this version is going to defer entirely to the CXXABI then presumably the other implementations of `getTailPaddingUseRules` should do that too - or all of this is to say maybe this property should stay in the ABI? & we should add the DefaultedSMFArePOD property to TargetCXXABI, instead of TargetInfo?
> 
> (all that said, I am somewhat confused by what an "OS" is, if it's not an ABI? What does it mean to use one OS but a different ABI? What features of the OS persist in that case? I can't think of any, but don't know much about these things)
I can't speak to any of that, it doesn't make any sense to me to separate the ABI from the OS.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135326/new/

https://reviews.llvm.org/D135326