Agreed that a test should be added.

I think it overall LGTM.

Thanks @dmgreen and @craig.topper for reviewing. :-)
I will fix the indentation in DAGCombiner on the commit, if that's okay.

• Update the patch with the latest master.
• Remove isMulhCheaperThanMulShift TLI Hook from all code segments.
• Update test/CodeGen/X86/pmulh.ll.

Overall I think this LGTM.

In D80485#2293955, @craig.topper wrote:

In D80485#2293942, @amyk wrote:

So, my goal was to introduce a DAG combine to combine multiply+shifts into mulh. This is done in a function I introduced within DAGCombiner.cpp (called combineShiftToMULH).

Since I was implementing something that is in target independent code, I thought it may be better to enable it on PowerPC only first since I was not quite sure if other targets were interested it this and I was seeing many other LIT failures at the time.
Thus, I introduced the hook (isMulhCheaperThanMulShift) and targets who wish to combine multiply+shifts into mulh could implement that hook. Within combineShiftToMULH, there is a check to isMulhCheaperThanMulShift.

If we wanted to enable this for everyone, I think the check isOperationLegalOrCustom would probably be sufficient. I just tried this patch again, and made the following changes for combineShiftToMULH:

@@ -8099,12 +8101,6 @@ static SDValue combineShiftToMULH(SDNode *N, SelectionDAG &DAG,
   if (NarrowVT !=  RightOp.getOperand(0).getValueType())
     return SDValue();

-  // Only transform into mulh if mulh for the narrow type is cheaper than
-  // a multiply followed by a shift. This should also check if mulh is
-  // legal for NarrowVT on the target.
-  if (!TLI.isMulhCheaperThanMulShift(NarrowVT))
-      return SDValue();
-
   // Proceed with the transformation if the wide type is twice as large
   // as the narrow type.
   unsigned NarrowVTSize = NarrowVT.getScalarSizeInBits();
@@ -8122,6 +8118,12 @@ static SDValue combineShiftToMULH(SDNode *N, SelectionDAG &DAG,
   // we use mulhs. Othewise, zero extends (zext) use mulhu.
   unsigned MulhOpcode = IsSignExt ? ISD::MULHS : ISD::MULHU;

+  // Only transform into mulh if mulh for the narrow type is cheaper than
+  // a multiply followed by a shift. This should also check if mulh is
+  // legal for NarrowVT on the target.
+  if (!TLI.isOperationLegalOrCustom(MulhOpcode, NarrowVT))
+      return SDValue();
+
   SDValue Result = DAG.getNode(MulhOpcode, DL, NarrowVT, LeftOp.getOperand(0),
                                RightOp.getOperand(0));
   return (N->getOpcode() == ISD::SRA ? DAG.getSExtOrTrunc(Result, DL, WideVT1)

I only see one LIT failure now:

Failed Tests (1):
  LLVM :: CodeGen/X86/pmulh.ll

Would this now be enabled on PPC32 or whatever !PPC64 is called? Are there tests for that?

LGTM.

So, my goal was to introduce a DAG combine to combine multiply+shifts into mulh. This is done in a function I introduced within DAGCombiner.cpp (called combineShiftToMULH).

• Updated the isEltOfVectorTy() to the correct semantics; making it take in a vector type and then getting the element type within the function.

Ping.

@nemanjai Would you please take another look to see if I have addressed your comments when you get a chance? Thanks.

Address Nemanja's review comments:

• More specific comments when bitcasting the inputs
• Pull out conditions to bitcast the input, use ternary op depending if the input is 32 or 64-bits
• Create new static function to check if a given type is the same type as a vector element

Updated identation/formatting of code in CGExprScalar.cpp and altivec.h.

Thanks for open coding. Aside from Nemanja's nits, I believe all my concerns have been addressed. LGTM.

Aside from the clang-format issues that can be addressed on commit this looks good to me.

Overall LGTM as well.

Forgot to approve this patch.

Address clang-format concerns.

Please address clang-format issues and Lei's comments. Other than that, I think the patch LGTM.

• Rebased patch.
• Update patch to remove unnecessary immediate handling.

In D87729#2275667, @Conanap wrote:

Looks like there's some unit test failures; could you double check?

A question I have is, is it possible for the 128-bit vector modulo instructions be open coded?

Update the run line of crbits.ll test.

@nemanjai Could you please take another look to see if I have addressed your comments?

Ping.

Ping.

Ping.

Update to address clang-format.

Ping.

Ping.

Update clang test names for vec_expandm.

Address review comments:

• Further consolidate the custom codegen of the two builtins
• Add SemaChecking for if the third argument is a constant, if the third argument is in range and if the second argument is the same type as the element type of the first argument
• Add extra test to test the semantic checks that were added

In D83560#2206363, @SouraVX wrote:

Thanks @MaskRay for pointers. Tried similar approach(slightly different, since expression has to be emitted by a separate streamer owned by DebugLocDwarfExpression) to take care of Endianess.

• Temporarily removed long double support since IMHO this might require a deeper surgery. Patch reflects the overall skeleton required for suporting DW_OP_implicit_value.
• @amyk, is it possible for you to try this patch on PPC targets ? So that we won't end up having surprises.

Ping.

Thanks for addressing the comments. LGTM.

I realized I didn't put a comment on this earlier but this overall LGTM, but I think it would be good to see if @nemanjai has any additional comments on this patch.

Overall LGTM.

Rebased patch, and removed MC tests from the original patch.

Rebased patch and removed MC tests from the patch.

Rebased the patch and removed MC tests.

I think overall it LGTM and the indentation can be addressed when committing.

Could we also elaborate in the description on how we are utilizing the new load instructions for zero extend case but not the sign extend case?

Thanks for recovering the tests. LGTM.

• Rebased patch
• Updated patch to remove instruction definitions and MC tests

Addressed the following comments:

• updated CHECK lines to check for the full intrinsic call
• updated indentation