[PATCH] D45146: [x86] Introduce a pass to begin more systematically fixing PR36028 and similar issues.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: llvm/lib/Target/X86/X86FlagsCopyLowering.cpp:615
+    Cond = X86::COND_B; // CF == 1
+    Addend = 127;
+    break;
----------------
chandlerc wrote:
> rnk wrote:
> > craig.topper wrote:
> > > I think these Addend values are backwards. 255 would work for carry, and 127 would work for overflow
> > I confirmed this with assembly. You might want to do some multi-precision ADDC execution tests locally to validate the change after fixing this.
> I tried sooooo many tests, included a bunch of high-precision arithmetic library tests... Nothing failed. =[ The only thing I've seen actually hit this code path is the mul-i1024.ll regression test ironically.
> 
> Maybe I can kill two birds with one stone by moving mul-i1024.ll to instead be a bitcode test of basic arithmetic. Anyways, any reason to not commit with this fixed?
Ok, I've now built a framework to let me add execution tests to the test-suite with arbitrary hand-written IR and written some high-precision integre multiply code that we know can hit this because we have a file-check test for it.

First problem, i have to do an i4096 x i4096 multiply to see EFLAGS get saved and restored on x86-64.

Second problem, I can't find an input to the multiply that produces wrong results. I know such an input exists, but only a relatively small number of carries in the multiply actually get hit with the bug. Finding the right input seems ... challenging.

Anyways, I'll clean up this execution test and land it. At the least it will let us get rid of a FileCheck based test for huge integer multiply lowering. SHould also show a good path to adding *very* reduced execution test cases which seems valuable.

I can add an execution test that *specifically* forces EFLAGS to be copied feeding adds with carries if you want, but it feels .... very narrow. Won't catch any future bugs.


Repository:
  rL LLVM

https://reviews.llvm.org/D45146