[ADT] Add APIntOps::mulhs / APIntOps::mulhu

[RKSimon]

• Move implementations from SelectionDAG.cpp FoldValue into APInt.h APIntOps
  

  llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

  Lines 6012 to 6016 in deff460

  	case ISD::MULHS: {
  	unsigned FullWidth = C1.getBitWidth() * 2;
  	APInt C1Ext = C1.sext(FullWidth);
  	APInt C2Ext = C2.sext(FullWidth);
  	return (C1Ext * C2Ext).extractBits(C1.getBitWidth(), C1.getBitWidth());

  
  

  llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

  Lines 6018 to 6022 in deff460

  	case ISD::MULHU: {
  	unsigned FullWidth = C1.getBitWidth() * 2;
  	APInt C1Ext = C1.zext(FullWidth);
  	APInt C2Ext = C2.zext(FullWidth);
  	return (C1Ext * C2Ext).extractBits(C1.getBitWidth(), C1.getBitWidth());

• Add APIntTest unit test coverage

• Update DivisionByConstantTest.cpp (this might require the lhs/rhs operands to be extended to the same bitwidth)

• Update KnownBitsTest.cpp to use APIntOps::mulhs/u in exhaustive tests

• Update mlir ArithOps.cpp + SPIRVCanonicalization.cpp to use APIntOps::mulhs/mulhu

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. In the comments of the issue, request for it to be assigned to you.
2. Fix the issue locally.
3. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
4. Create a Git commit.
5. Run git clang-format HEAD~1 to format your changes.
6. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: Simon Pilgrim (RKSimon)

- [ ] Move implementations from SelectionDAG.cpp FoldValue into APInt.h APIntOps https://github.com/llvm/llvm-project/blob/deff460b46dfcc8d6d5917a2b78c0d52edbe4afb/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp#L6012-L6016 https://github.com/llvm/llvm-project/blob/deff460b46dfcc8d6d5917a2b78c0d52edbe4afb/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp#L6018-L6022

• Add APIntTest unit test coverage
• Update DivisionByConstantTest.cpp
• Update KnownBitsTest.cpp to use APIntOps::mulhs/u in exhaustive tests

[Atousa]

@RKSimon I am interested in working on this issue. Please assign it to me.

[EugeneZelenko]

@Atousa: Just create pull request and mention it on this page.

[Sh0g0-1758]

Hey there, I am also interested in working on this. I will link a PR soon. @Atousa , since you are assigned the issue, if you have not done significant work on this, can we collaborate?

[Atousa]

@RKSimon @Sh0g0-1758 this is just assigned to me and I am working on it. This is not collaborative!

[Sh0g0-1758]

Hm... I saw no PR for 3 days and the solution was rather simple, so thought of working on it. Anyways, if you do not wish to collaborate, that is fine. Please link a PR and I will close mine. Though, @RKSimon, I suppose there is no harm in 2 people working on this issue. P.S., the checks are passing.

[Sh0g0-1758]

Just out of curiosity, @RKSimon, in the 4th task, you mentioned updating KnownBitsTest.cpp but In it, we are using KnownBits::mul. I don't think I was supposed to update it, so could you please clarify as to what you meant?

[RKSimon]

Just out of curiosity, @RKSimon, in the 4th task, you mentioned updating KnownBitsTest.cpp but In it, we are using KnownBits::mul. I don't think I was supposed to update it, so could you please clarify as to what you meant?

I meant the APInt reference version in the exhaustive tests

llvm-project/llvm/unittests/Support/KnownBitsTest.cpp

Lines 540 to 543 in deff460

	[](const APInt &N1, const APInt &N2) {
	unsigned Bits = N1.getBitWidth();
	return (N1.sext(2 * Bits) * N2.sext(2 * Bits)).extractBits(Bits, Bits);
	},

[RKSimon]

BTW @Sh0g0-1758 I thought @Atousa was working on this ticket?

[Sh0g0-1758]

Well yeah, she still can, I don't mind closing my PR, but I do believe that most of the work is done. I guess this sums it up : #84207 (comment)

[RKSimon]

OK - 3 days is a bit short, especially for good first issues where new contributors might still be getting up to speed.

We typically recommend at least 7 days between pings on reviews that have gone quiet, we should probably have a similar recommendation here.

Glad you're keen to help :)

[Sh0g0-1758]

Ah sure thing. Will keep that in mind from now on.

[Atousa]

BTW @Sh0g0-1758 I thought @Atousa was working on this ticket?

Yes I am woking on it.

[Atousa]

@RKSimon @jayfoad @kuhar , this task has been assigned to me. As we try to foster a healthy collaborative community within LLVM, it's crucial that we adhere to the code of conduct. I'm puzzled by why this individual continues to work on the task despite my clear communication that it's not meant to be a collaborative effort. I intend to bring this matter to @lattner @AaronBallman's attention to prevent similar incidents from occurring in the future within the LLVM community. Please continue the conversation on this matter here: #80939

[RKSimon]

@Atousa I am sorry about this - I missed the author username when I first started reviewing the PR and it has snowballed since then. The issue was assigned to you and @Sh0g0-1758 was hasty in trying to take over. My advice is to get on with your own version of the PR.

[Sh0g0-1758]

No worries. I will close my PR. Sorry for the inconvenience.

[RKSimon]

Thank you.

I had not expected the amount of interest in the good-first-issue tickets that I created, in the past such tickets have been ignored for months, my advice is in future to ensure that you have been tagged as assignee before beginning anything more substantive than triage work on a ticket. I intend to create more good-first-issue tickets as there is obviously interest at the moment, I just ask that everyone works on only one (or two) issues at once - please don't accumulate too many assigned tickets at once.

@Atousa was the assignee for #84207 and should be given a reasonable amount of time to create their own PR - as I said previously, typically about 7 days to respond to 'ping' request-for-update messages.

[Sh0g0-1758]

Yes, I understand. I just saw it as an easy fix and gave a patch. I guess what really went wrong was my misinterpretation of the below message as an approval for my view on the topic ie. To go ahead with my PR but to keep this in mind in the future.

OK - 3 days is a bit short, especially for good first issues where new contributors might still be getting up to speed.

We typically recommend at least 7 days between pings on reviews that have gone quiet, we should probably have a similar recommendation here.

Glad you're keen to help :)