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

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp:396
+  // The stack pointer (R1) is not clobberable by inline asm
+  return PhysReg != PPC::R1;
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112073