[PATCH] D31360: [GlobalISel]: Simple helper functions used across GISel pipeline

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/Utils.h:65
+/// Check whether \p Opc is floating point opcode
+bool isFloatingPointOpc(unsigned Opc);
+
----------------
ab wrote:
> This name is too generic to live in the llvm namespace  (I would expect the function to take Instruction:: opcodes).
> 
> More importantly, AArch64RegisterBankInfo.cpp has a 'isPreISelGenericFloatingPointOpcode', which has a different behavior (it ignores itofp, because it interprets 'floating point opcode' as 'all operands are FP values')
> 
> IMO that shows that this is too nebulous to be shared.  Keep it next to your use?
> 
> Otherwise, maybe we could make this precise enough that we could share it?  For instance, 'isFloatingPointVRegDef' that takes a vreg, checks its definition, and returns true if the def is known to be FP?
Yup - The precise definition would work. Should I rename that helper function and change the behavior to take a vreg instead?


================
Comment at: include/llvm/CodeGen/GlobalISel/Utils.h:76
+/// Helper function to get integer value from \p MI at \p Idx
+uint64_t getConstantOperandVal(const MachineInstr &MI, unsigned Idx,
+                               const MachineRegisterInfo &MRI);
----------------
ab wrote:
> I added a 'getConstantVRegVal' in r298855, but that's only in the selector;  the Optional hopefully lets it replace all three functions (after extracting it from the selector) ?
Yes - getConstantVRegVal would totally replace the above functions. I'll update the backend code to use that instead.


Repository:
  rL LLVM

https://reviews.llvm.org/D31360