[PATCH] D108928: [X86ISelLowering] avoid emitting libcalls to __mulodi4()

Tue Aug 31 14:14:57 PDT 2021

nickdesaulniers added inline comments.

================
Comment at: llvm/test/CodeGen/X86/overflow-intrinsic-optimizations.ll:1
+; RUN: llc %s -mtriple=i386 -o - | FileCheck %s
+
----------------
craig.topper wrote:
> nickdesaulniers wrote:
> > RKSimon wrote:
> > > Please use the update_llc_test_checks.py script to generate checks
> > Honestly, update_llc_test_checks.py is a big hammer. It's going to add 84 lines to this test, which simply checks we don't call one function `__mulodi4`. I don't care what the resulting code is, as long as we don't call the function.  I've added it as you asked, but I think it significantly hurts the readability of this test.
> > 
> > It's convenient for sure, but it ends up generating significant churn in the tests, and reviewers don't need to think about why a test was important or whether they're changing a test for the worst; they can just run an automated tool until the test passes; who cares what the original intent of the test ever was and whether we just steam-rolled it?
> I think I agree with Nick here. The only value this test has over xmulo.ll is that the test is named no__mulodi4. The -NOT was the most straightforward way to check exactly what the intent of this test is. With the script being run on it we lose that and might as well not have it and rely on xmulo.ll
That's reasonable; in that case @RKSimon would you prefer I either:
1. delete this newly added test, llvm/test/CodeGen/X86/overflow-intrinsic-optimizations.ll and rely on the other modified tests for coverage?
2. revert llvm/test/CodeGen/X86/overflow-intrinsic-optimizations.ll back to 369497?

(I guess there's also 3. commit code as is...)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108928