[PATCH] D56057: [X86] Individually simplify both operands of PMULDQ/PMULUDQ using the other entry point of SimplifyDemandedBits that allows the one use check of the root node to be suppressed.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

In D56057#1340475 <https://reviews.llvm.org/D56057#1340475>, @easyaspi314 wrote:

> I am a little suspicious about these changes to the test results.
>
> 1. If these removals are all of redundant instructions that were added previously, I worry about how well people are checking the test result output. A large chunk of the code we are removing is from this: https://reviews.llvm.org/rL347181. As you can see, there are major changes to the test results, which, unless I am mistaken, passed without questioning whether these changes are a regression or an improvement. If nobody checks test results for regressions, why have tests at all?


I think you'e referring to pmul.ll?  rL347181 <https://reviews.llvm.org/rL347181> just replaced psrad $31 with pcmpgt. It changed the register allocation quite a bit which made the diff look large. But it didn't change the number of instructions other than copies and xors.

> 2. I also worry that they might not return the correct result. I am especially concerned about the number of removed i64 vector multiplies.

I'll take another look.

> 3. Otherwise, if these all return the correct result, nice job.





================
Comment at: lib/Target/X86/X86ISelLowering.cpp:41278
+  APInt DemandedMask = APInt::getLowBitsSet(64, 32);
+  {
+    TargetLowering::TargetLoweringOpt TLO(DAG, !DCI.isBeforeLegalize(),
----------------
RKSimon wrote:
> Is it worth moving this code inside SimplifyDemandedBitsForTargetNode?
> 
> Wrap this in a for loop to reduce code?
> 
> ```
> for {int OpIdx  = 0; OpIdx != 2; ++OpIdx) {
>     TargetLowering::TargetLoweringOpt TLO(DAG, !DCI.isBeforeLegalize(),
>                                           !DCI.isBeforeLegalizeOps());
>     if (TLI.SimplifyDemandedBits(N, OpIdx, DemandedMask, DCI, TLO)) {
>       DCI.AddToWorklist(N);
>       return SDValue(N, 0);
>     }
> }
> ```
I don't think that will work given the current implementation AssumeSingleUse. The code that does the replacement assumes that a node with multiple uses is the root node of the simplification.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56057