[PATCH] D124415: [PowerPC][NFC] Add a function to determine if a call needs to be NOTOC.

[ChenZheng via Phabricator via llvm-commits]

shchenz accepted this revision as: shchenz.
shchenz added a comment.

LGTM with one nit.



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.h:299
+  /// Check if Opcode corresponds to a call instruction that should be marked
+  /// with the NOTOC relocation.
+  bool isNoTOCCallInstr(unsigned Opcode) const {
----------------
jsji wrote:
> shchenz wrote:
> > jsji wrote:
> > > shchenz wrote:
> > > > I am in favor of Jinsong's comment in https://reviews.llvm.org/D122012#3392673. Could we use a flag in the td file (with a default true/false value) instead of listing all call instructions in the cpp file?
> > > Thanks Zheng, we had some offline discussion, and I agreed that this can be a tradeoff between cost and benefits.
> > > So I am OK with this. 
> > Oh, I don't realize this. Could you please explain why we choose the cpp function instead of a td flag? To me, when we add a new call instruction in td file in future, it should be easier to aware that there is a flag to tell whether this call instruction needs a nop or not. Thanks
> We will need some big in `TSFlags` to check the td flag, and currently this only affects 3 opcode, and it is highly unlikely we will be adding more `NOTOC` call instructions. 
> So this explicitly listed switch table is mostly as a tradeoff of saving bits in `TSFlags`
OK, thanks for explanation.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.h:314
+      return true;
+    case PPC::BL8:
+    case PPC::BL:
----------------
nit: maybe we can also guard these false cases under `#ifndef NDEBUG`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124415