[SelectionDAG][Mips][Sparc] Don't allow SimplifyDemandedBits to constant fold TargetConstant nodes to a Constant.

[craig.topper]

Summary

After the switch in SimplifyDemandedBits, it tries to create a
constant when possible. If the original node is a TargetConstant
the default in the switch will call computeKnownBits on the
TargetConstant which will succeed. This results in the
TargetConstant becoming a Constant. But TargetConstant exists to
avoid being changed.

I've fixed the two cases that relied on this in tree by explicitly
making the nodes constant instead of target constant. The Sparc
case is an old bug. The Mips case was recently introduced now that
ImmArg on intrinsics gets turned into a TargetConstant when the
SelectionDAG is created.

[jyknight]

Sparc change looks good.
Don't know about the MIPS one -- seems like probably something upstream should be creating a Constant instead of a TargetConstant, rather than converting it there?

[arsenm]

llvm/lib/Target/Mips/MipsSEISelLowering.cpp

2302–2303 ↗

(On Diff #220958)

This one looks like it's using immarg for an intrinsic argument where it really shouldn't be immarg

[arsenm]

llvm/lib/Target/Mips/MipsSEISelLowering.cpp

2302–2303 ↗

(On Diff #220958)

I fixed another such case in r356085; this looks like another intrinsic that ends up lowered to generic DAG opcodes so it doesn't really have the restriction

2302–2303 ↗

(On Diff #220958)

r366328 rather

[craig.topper]

llvm/lib/Target/Mips/MipsSEISelLowering.cpp

2302–2303 ↗

(On Diff #220958)

Should we remove the I from the builtin declaration in the frontend too?

[craig.topper]

Remove ImmArg from Mips intrinsics

[arsenm]

llvm/lib/Target/Mips/MipsSEISelLowering.cpp

2302–2303 ↗

(On Diff #220958)

I would probably just remove the intrinsic and emit the plain IR for compatibility. I don't see the point of having intrinsics that lower to generic operations

[arsenm]

LGTM but could use tests

[craig.topper]

In D67802#1676910, @arsenm wrote:

LGTM but could use tests

How would you test this? The issues fixed fail existing tests with the llvm_unreachable added to SimplifyDemandedBits.

[arsenm]

In D67802#1676937, @craig.topper wrote:

In D67802#1676910, @arsenm wrote:

LGTM but could use tests

How would you test this? The issues fixed fail existing tests with the llvm_unreachable added to SimplifyDemandedBits.

oh, ok

[thakis]

This breaks this test:

[0/2] ACTION //llvm/test:check-llvm(//llvm/utils/gn/build/toolchain:unix)
-- Testing: 33653 tests, 64 threads --
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90
FAIL: LLVM :: Verifier/Mips/intrinsic-immarg.ll (31185 of 33653)
******************** TEST 'LLVM :: Verifier/Mips/intrinsic-immarg.ll' FAILED ********************
Script:
--
: 'RUN: at line 1';   not /usr/local/google/home/thakis/src/llvm-project/out/gn/bin/llvm-as /usr/local/google/home/thakis/src/llvm-project/llvm/test/Verifier/Mips/intrinsic-immarg.ll -o /dev/null 2>&1 | /usr/local/google/home/thakis/src/llvm-project/out/gn/bin/FileCheck /usr/local/google/home/thakis/src/llvm-project/llvm/test/Verifier/Mips/intrinsic-immarg.ll
--
Exit Code: 2

Command Output (stderr):
--
FileCheck error: '-' is empty.
FileCheck command line:  /usr/local/google/home/thakis/src/llvm-project/out/gn/bin/FileCheck /usr/local/google/home/thakis/src/llvm-project/llvm/test/Verifier/Mips/intrinsic-immarg.ll

llvm/trunk/include/llvm/IR/IntrinsicsMips.td

Intrinsic<[llvm_v4i32_ty],
             [IntrNoMem, ImmArg<1>]>;
 def int_mips_insve_d : GCCBuiltin<"__builtin_msa_insve_d">,
   Intrinsic<[llvm_v2i64_ty],
             [llvm_v2i64_ty, llvm_i32_ty, llvm_v2i64_ty],
             [IntrNoMem, ImmArg<1>]>;
 
 def int_mips_ld_b : GCCBuiltin<"__builtin_msa_ld_b">,
   Intrinsic<[llvm_v16i8_ty], [llvm_ptr_ty, llvm_i32_ty],
-  [IntrReadMem, IntrArgMemOnly, ImmArg<1>]>;
+  [IntrReadMem, IntrArgMemOnly]>;
 def int_mips_ld_h : GCCBuiltin<"__builtin_msa_ld_h">,
   Intrinsic<[llvm_v8i16_ty], [llvm_ptr_ty, llvm_i32_ty],
-  [IntrReadMem, IntrArgMemOnly, ImmArg<1>]>;
+  [IntrReadMem, IntrArgMemOnly]>;
 def int_mips_ld_w : GCCBuiltin<"__builtin_msa_ld_w">,
   Intrinsic<[llvm_v4i32_ty], [llvm_ptr_ty, llvm_i32_ty],
-  [IntrReadMem, IntrArgMemOnly, ImmArg<1>]>;
+  [IntrReadMem, IntrArgMemOnly]>;
 def int_mips_ld_d : GCCBuiltin<"__builtin_msa_ld_d">,
   Intrinsic<[llvm_v2i64_ty], [llvm_ptr_ty, llvm_i32_ty],
-  [IntrReadMem, IntrArgMemOnly, ImmArg<1>]>;
+  [IntrReadMem, IntrArgMemOnly]>;
 
 def int_mips_ldi_b : GCCBuiltin<"__builtin_msa_ldi_b">,
   Intrinsic<[llvm_v16i8_ty], [llvm_i32_ty], [IntrNoMem, ImmArg<0>]>;
 def int_mips_ldi_h : GCCBuiltin<"__builtin_msa_ldi_h">,
   Intrinsic<[llvm_v8i16_ty], [llvm_i32_ty], [IntrNoMem, ImmArg<0>]>;
 def int_mips_ldi_w : GCCBuiltin<"__builtin_msa_ldi_w">,
   Intrinsic<[llvm_v4i32_ty], [llvm_i32_ty], [IntrNoMem, ImmArg<0>]>;
 def int_mips_ldi_d : GCCBuiltin<"__builtin_msa_ldi_d">,
def int_mips_srlri_h : GCCBuiltin<"__builtin_msa_srlri_h">,
   Intrinsic<[llvm_v8i16_ty], [llvm_v8i16_ty, llvm_i32_ty], [IntrNoMem, ImmArg<1>]>;
 def int_mips_srlri_w : GCCBuiltin<"__builtin_msa_srlri_w">,
   Intrinsic<[llvm_v4i32_ty], [llvm_v4i32_ty, llvm_i32_ty], [IntrNoMem, ImmArg<1>]>;
 def int_mips_srlri_d : GCCBuiltin<"__builtin_msa_srlri_d">,
   Intrinsic<[llvm_v2i64_ty], [llvm_v2i64_ty, llvm_i32_ty], [IntrNoMem, ImmArg<1>]>;
 
 def int_mips_st_b : GCCBuiltin<"__builtin_msa_st_b">,
   Intrinsic<[], [llvm_v16i8_ty, llvm_ptr_ty, llvm_i32_ty],
-  [IntrArgMemOnly, ImmArg<2>]>;
+  [IntrArgMemOnly]>;
 def int_mips_st_h : GCCBuiltin<"__builtin_msa_st_h">,
   Intrinsic<[], [llvm_v8i16_ty, llvm_ptr_ty, llvm_i32_ty],
-  [IntrArgMemOnly, ImmArg<2>]>;
+  [IntrArgMemOnly]>;
 def int_mips_st_w : GCCBuiltin<"__builtin_msa_st_w">,
   Intrinsic<[], [llvm_v4i32_ty, llvm_ptr_ty, llvm_i32_ty],
-  [IntrArgMemOnly, ImmArg<2>]>;
+  [IntrArgMemOnly]>;
 def int_mips_st_d : GCCBuiltin<"__builtin_msa_st_d">,
   Intrinsic<[], [llvm_v2i64_ty, llvm_ptr_ty, llvm_i32_ty],
-  [IntrArgMemOnly, ImmArg<2>]>;
+  [IntrArgMemOnly]>;
 
 def int_mips_subs_s_b : GCCBuiltin<"__builtin_msa_subs_s_b">,
   Intrinsic<[llvm_v16i8_ty], [llvm_v16i8_ty, llvm_v16i8_ty], [IntrNoMem]>;
 def int_mips_subs_s_h : GCCBuiltin<"__builtin_msa_subs_s_h">,
   Intrinsic<[llvm_v8i16_ty], [llvm_v8i16_ty, llvm_v8i16_ty], [IntrNoMem]>;
 def int_mips_subs_s_w : GCCBuiltin<"__builtin_msa_subs_s_w">,
   Intrinsic<[llvm_v4i32_ty], [llvm_v4i32_ty, llvm_v4i32_ty], [IntrNoMem]>;
 def int_mips_subs_s_d : GCCBuiltin<"__builtin_msa_subs_s_d">,

llvm/trunk/lib/CodeGen/SelectionDAG/TargetLowering.cpp

if (!Op.getNode()->hasOneUse() && !AssumeSingleUse) {
     return TLO.CombineTo(Op, TLO.DAG.getUNDEF(VT));
   } else if (Depth >= SelectionDAG::MaxRecursionDepth) {
     // Limit search depth.
     return false;
   }
 
   KnownBits Known2, KnownOut;
   switch (Op.getOpcode()) {
+  case ISD::TargetConstant:
+    llvm_unreachable("Can't simplify this node");
   case ISD::SCALAR_TO_VECTOR: {
     if (!DemandedElts[0])
       return TLO.CombineTo(Op, TLO.DAG.getUNDEF(VT));
 
     KnownBits SrcKnown;
     SDValue Src = Op.getOperand(0);
     unsigned SrcBitWidth = Src.getScalarValueSizeInBits();
     APInt SrcDemandedBits = DemandedBits.zextOrSelf(SrcBitWidth);

llvm/trunk/lib/Target/Sparc/SparcISelLowering.cpp

SDValue SparcTargetLowering::LowerF128Compare(SDValue LHS, SDValue RHS,
 
   switch(SPCC) {
   default: {
     SDValue RHS = DAG.getTargetConstant(0, DL, Result.getValueType());
     SPCC = SPCC::ICC_NE;
     return DAG.getNode(SPISD::CMPICC, DL, MVT::Glue, Result, RHS);
   }
   case SPCC::FCC_UL : {
-    SDValue Mask   = DAG.getTargetConstant(1, DL, Result.getValueType());
+    SDValue Mask   = DAG.getConstant(1, DL, Result.getValueType());
     Result = DAG.getNode(ISD::AND, DL, Result.getValueType(), Result, Mask);
     SDValue RHS    = DAG.getTargetConstant(0, DL, Result.getValueType());
     SPCC = SPCC::ICC_NE;
     return DAG.getNode(SPISD::CMPICC, DL, MVT::Glue, Result, RHS);
   }
   case SPCC::FCC_ULE: {
     SDValue RHS = DAG.getTargetConstant(2, DL, Result.getValueType());
     SPCC = SPCC::ICC_NE;
case SPCC::FCC_U  :  {
     return DAG.getNode(SPISD::CMPICC, DL, MVT::Glue, Result, RHS);
   }
   case SPCC::FCC_O  :  {
     SDValue RHS = DAG.getTargetConstant(3, DL, Result.getValueType());
     SPCC = SPCC::ICC_NE;
     return DAG.getNode(SPISD::CMPICC, DL, MVT::Glue, Result, RHS);
   }
   case SPCC::FCC_LG :  {
-    SDValue Mask   = DAG.getTargetConstant(3, DL, Result.getValueType());
+    SDValue Mask   = DAG.getConstant(3, DL, Result.getValueType());
     Result = DAG.getNode(ISD::AND, DL, Result.getValueType(), Result, Mask);
     SDValue RHS    = DAG.getTargetConstant(0, DL, Result.getValueType());
     SPCC = SPCC::ICC_NE;
     return DAG.getNode(SPISD::CMPICC, DL, MVT::Glue, Result, RHS);
   }
   case SPCC::FCC_UE : {
-    SDValue Mask   = DAG.getTargetConstant(3, DL, Result.getValueType());
+    SDValue Mask   = DAG.getConstant(3, DL, Result.getValueType());
     Result = DAG.getNode(ISD::AND, DL, Result.getValueType(), Result, Mask);
     SDValue RHS    = DAG.getTargetConstant(0, DL, Result.getValueType());
     SPCC = SPCC::ICC_E;
     return DAG.getNode(SPISD::CMPICC, DL, MVT::Glue, Result, RHS);
   }
   }
 }