[PATCH] D112073: [PowerPC] Emit warning when SP is clobbered by asm

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

This needs more work.

1. We need a clang test case similar to `clang/test/Misc/inline-asm-clobber-warning.c`
2. We need to handle `r1` on both 64-bit and 32-bit because `x1` is just an internal LLVM name
3. We need to handle `sp` as an alias for `r1` as GCC does



================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp:396
+  // The stack pointer (R1) is not clobberable by inline asm
+  return PhysReg != PPC::R1;
+}
----------------
shchenz wrote:
> nemanjai wrote:
> > shchenz wrote:
> > > PowerPC overrides `getReservedRegs()`, why don't we use this function and only warn for `R1` even no `X1`?
> > It is unfortunately not as simple as that. We do have some reserved registers that can be clobbered. For example `R2` can be clobbered since the compiler will spill and restore it.
> > There should be a comment to that end in the code. The test should definitely cover `X1` in 64-bit mode.
> > 
> > Furthermore, there needs to be a 64-bit test as well.
> Only marking `R1/X1` as reserved for PowerPC should be working. But I still think using `getReservedRegs()` is a better way. If we reserve some registers which can be allocated for some cases conservatively, maybe we should change the logic in `getReservedRegs`? For the `R2/X2` example, I saw there is a `TODO` to make it allocatable.
> 
> And even we are aggressive to mark `R2/X2`(all registers in `getReservedRegs()`) as reserved, we have to choose which one of the below two cases is worse:
> 1: aggressively mark all registers in `getReservedRegs()` as reserved. We may give a false warning about the clobber register in the inline assembly. For example, we should be able to clobber `R2/X2` in a function that does not access TOC.
> 2: We only emit warnings for `R1/X1` like this patch does. We may miss giving a warning for other reserved registers, for example `R13/X13`. (I saw a real-world case for this wrong usage of thread pointer in the inline assembly).
> 
> Anyhow I will not be a blocker for this patch to get in as it is also right.
While I certainly agree that only focusing on R1 is a bit problematic, false warnings aren't free. There are definitely projects that use inline asm and build with `-Werror`. Breaking such projects would definitely be an undesirable outcome.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112073