Page MenuHomePhabricator

[SeparateConstOffsetFromGEP][PowerPC] Fix: sext(a) + sext(b) -> sext(a + b) matches add and sub instructions with one another
ClosedPublic

Authored by ajwock on Aug 8 2019, 1:06 PM.

Details

Summary

During the SeparateConstOffsetFromGEP pass, signed extensions are distributed and then later recombined. The recombination stage is somewhat problematic- it doesn't differ add and sub instructions from another when matching the sext(a) +/- sext(b) -> sext(a +/- b) pattern, although this issue is incredibly rare. It only occurs under the below conditions. The test case only miscompiles on PowerPC as per my current testing; however, the issue is not in PowerPC specific code as far as I can tell, and it appears that the right IR input could theoretically trigger this on multiple platforms.

The IR contains:
%unextendedA
%unextendedB
%subuAuB = unextendedA - unextendedB
%extA = extend A
%extB = extend B
%addeAeB = extA + extB

The problematic code will transform that into:

%unextendedA
%unextendedB
%subuAuB = unextendedA - unextendedB
%extA = extend A
%extB = extend B
%addeAeB = extend subuAuB ; Obviously not semantically equivalent to the IR input.

The operands must be in the same order as above- and other optimization passes must preserve this behavior until this section of the code is reached, which is unlikely with the possibly-unnecessary extensions. My test case was heavily reduced both at the C level and the IR level from cblas_ztrsv, but there are many extraneous instructions creating dependencies necessary to preserve the miscompilation.

I do appreciate your documentation, jingyue.

Diff Detail

Event Timeline

ajwock created this revision.Aug 8 2019, 1:06 PM
ajwock added a comment.Aug 8 2019, 1:08 PM

Diff did not include my test case- correcting that.

test coverage missing.

ajwock updated this revision to Diff 214216.Aug 8 2019, 1:11 PM

Hi, I am not familiar with this code but seeing as you're not getting reviews and this seems like a serious problem, I am willing to take a look (and maybe if I say something wrong here, Cunningham's Law will save us).

Without looking at the code too deeply, I am wondering about the testcase. Per the commit message it sounds like you're saying that invalid LLVM IR is generated. If so, can we have an LLVM IR testcase that runs this specific pass, rather than an llc testcase? Based on what's in the commit message, it sounds like such a testcase would be far simpler (and indeed less fragile) than what we have here.

arsenm added inline comments.Oct 25 2019, 1:35 PM
test/Transforms/SeparateConstOffsetFromGEP/PowerPC/test-nomatch-add-and-sub.ll
8 ↗(On Diff #214216)

I'm not opposed to including this testcase, but I would like to have a pure IR one that only runs the one pass

ajwock updated this revision to Diff 227494.Nov 1 2019, 11:26 AM

Sorry for the delay- I had to investigate some of the fragility so that my test case reduced to this pass would still trigger the issue. However, I did manage to get a test case that is only a few lines long that would trigger the issue without the update.

Sorry for the delay- I had to investigate some of the fragility so that my test case reduced to this pass would still trigger the issue. However, I did manage to get a test case that is only a few lines long that would trigger the issue without the update.

I suppose now that the test case is not PPC specific, it can be moved to a different directory?

ajwock updated this revision to Diff 238032.Jan 14 2020, 10:25 AM

Changed directory.

arsenm accepted this revision.Jan 15 2020, 6:07 AM

LGTM. I'm not super familiar with this code, but this seems reasonable. There's nothing new going on, and just splitting add and sub handling

This revision is now accepted and ready to land.Jan 15 2020, 6:07 AM
kpn added a comment.Jan 15 2020, 7:30 AM

Excellent! Thanks Matt!

Drew will be back in the office tomorrow. I'll commit this for him then.

This revision was automatically updated to reflect the committed changes.