[PATCH] D99439: Update @llvm.powi to handle different int sizes for the exponent

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/docs/LangRef.rst:13280
+floating-point or vector of floating-point type. The type of the exponent
+should typically match the size of ``int``, at least when the intrinsic maps
+to one of the ``__powi*`` functions in compiler-rt. Not all targets support
----------------
fhahn wrote:
> Not sure about the sentence about the size of the exponent. It refers to `int` and `__powi`, both of which are not really defined in the LangRef and it is not clear to what they are referring to here.
I see your point. Maybe something like "The type of the exponent should match the libm implementation for a target that lower the intrinsic to such a libcall.". Or maybe I should just skip trying to say something about it here.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:573
   unsigned Offset = IsStrict ? 1 : 0;
-  assert(N->getOperand(1 + Offset).getValueType() == MVT::i32 &&
+  assert((N->getOperand(1 + Offset).getValueType() == MVT::i16 ||
+          N->getOperand(1 + Offset).getValueType() == MVT::i32) &&
----------------
fhahn wrote:
> Why just allow i32 and i16 here?
> 
> Could we restrict the supported offset types to i32 or smaller?
I think that i16 and i32 are the only sizes of "int" that are supported by any in-tree target. And people tend to get very upset if trying to generalize code outside the scope given by in-tree targets.


================
Comment at: llvm/test/Transforms/InstCombine/pow-4.ll:4
+; RUN: opt -instcombine -S < %s -mtriple unknown -disable-builtin sqrt  | FileCheck %s --check-prefixes=CHECK,CHECKI32,NOSQRT
+; RUN: opt -instcombine -S < %s -mtriple msp430                        | FileCheck %s --check-prefixes=CHECK,CHECKI16,SQRT
+; RUN: opt -instcombine -S < %s -mtriple msp430 -disable-builtin sqrt  | FileCheck %s --check-prefixes=CHECK,CHECKI16,NOSQRT
----------------
Need to move these tests to a msp430 subdir.


================
Comment at: llvm/test/Transforms/InstCombine/pow_fp_int16.ll:1
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt -mtriple unknown -instcombine -S < %s | FileCheck %s
+; RUN: opt -mtriple msp430 -instcombine -S < %s | FileCheck %s
 
----------------
fhahn wrote:
> I don't think we can rely on any specific triple here. If a triple is needed it should be in a sub-directory.
Yes, right, I need to move this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99439