Page MenuHomePhabricator

[AMDGPU] Avoid DAG combining assert with fneg(fadd(A,0))
ClosedPublic

Authored by tpr on Apr 12 2019, 1:14 PM.

Details

Summary

fneg combining attempts to turn it into fadd(fneg(A), fneg(0)), but
creating the new fadd folds to just fneg(A). When A has multiple uses,
this confuses it and you get an assert. Fixed.

Change-Id: I0ddc9b7286abe78edc0cd8d734fdeb05ff09821c

Diff Detail

Repository
rL LLVM

Event Timeline

tpr created this revision.Apr 12 2019, 1:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2019, 1:14 PM

That's the best bugpoint could do with the test.

arsenm added inline comments.Apr 12 2019, 1:27 PM
test/CodeGen/AMDGPU/assert-combine-fneg-fadd-0.ll
8–39 ↗(On Diff #194945)

I can usually go a lot further than bug point, especially once I know what the actual problem is. This is also going to only hit one of the cases you added here.

The test cases should also go in the existing file for these combines

tpr updated this revision to Diff 195116.Apr 15 2019, 3:03 AM

V2: Cut down test a bit more and put it in fneg-combines.ll.

tpr added a comment.Apr 15 2019, 3:06 AM

I have cut down the test a bit more and put it into fneg-combines.ll. I did not manage to repro any problems with the other cases that I added fixes for.

I don't have a massive amount of time to spend on this. Would you prefer I land it as is, without tests for the other cases, or remove the precautionary fix for those cases and wait for someone else to possibly encounter them?

In D60633#1466195, @tpr wrote:

I have cut down the test a bit more and put it into fneg-combines.ll. I did not manage to repro any problems with the other cases that I added fixes for.

I don't have a massive amount of time to spend on this. Would you prefer I land it as is, without tests for the other cases, or remove the precautionary fix for those cases and wait for someone else to possibly encounter them?

In either case, you should open a bug somewhere to look at this again.

lib/Target/AMDGPU/AMDGPUISelLowering.cpp
3715–3716 ↗(On Diff #195116)

I think it would be slightly safer to check != ISD::FADD, in case some other fold decided to do something else

test/CodeGen/AMDGPU/fneg-combines.ll
219 ↗(On Diff #195116)

Should check something

tpr updated this revision to Diff 195183.Apr 15 2019, 7:53 AM

V3: Addressed review comments.

tpr marked 2 inline comments as done.Apr 15 2019, 7:54 AM

Bug for adding remaining test cases is https://bugs.llvm.org/show_bug.cgi?id=41500

arsenm accepted this revision.Apr 17 2019, 4:59 AM

LGTM

This revision is now accepted and ready to land.Apr 17 2019, 4:59 AM
This revision was automatically updated to reflect the committed changes.