Page MenuHomePhabricator

[InstCombine] Cleanup the TFE/LWE check in AMDGPU SimplifyDemanded
ClosedPublic

Authored by nhaehnle on Feb 4 2019, 5:02 AM.

Details

Summary

The fix added in r352904 is not quite correct, or rather misleading:

  1. When the texfailctrl (TFC) argument was non-constant, the fix assumed non-TFE/LWE, which is incorrect.
  1. Regardless, this code path cannot even be hit for correct TFE/LWE-enabled calls, because those return a struct. Added a test case for those for completeness.

Change-Id: I92d314dbc67a2670f6d7adaab765ef45f56a49cf

Diff Detail

Repository
rL LLVM

Event Timeline

nhaehnle created this revision.Feb 4 2019, 5:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2019, 5:02 AM
Herald added subscribers: t-tye, tpr, yaxunl and 3 others. · View Herald Transcript
This revision is now accepted and ready to land.Feb 4 2019, 5:14 AM
arsenm added a comment.Feb 4 2019, 5:37 AM

Does it actually matter? I thought since this needs to be a constant, this just needs to not crash

test/Transforms/InstCombine/AMDGPU/amdgcn-demanded-vector-elts.ll
2420 ↗(On Diff #185028)

There's an _ in the name?

hliao accepted this revision.Feb 4 2019, 11:42 AM

LGTM

nhaehnle marked an inline comment as done.Feb 4 2019, 1:11 PM

Does it actually matter? I thought since this needs to be a constant, this just needs to not crash

It's certainly a bit on the paranoid side. I was considering the possibility that somebody passes a value that only becomes constant after inlining.

test/Transforms/InstCombine/AMDGPU/amdgcn-demanded-vector-elts.ll
2420 ↗(On Diff #185028)

You mean the sl_v4f32i32s part? That's what the intrinsic name got canonicalized to for some reason. I'm not really uptodate on the mangling of struct types...

arsenm added a comment.Feb 4 2019, 1:15 PM

Does it actually matter? I thought since this needs to be a constant, this just needs to not crash

It's certainly a bit on the paranoid side. I was considering the possibility that somebody passes a value that only becomes constant after inlining.

I've been hoping to start working soon on a way to define which intrinsic operands are required to be constants to avoid hacks in global isel, so hopefully we won't need to worry about this much longer.

test/Transforms/InstCombine/AMDGPU/amdgcn-demanded-vector-elts.ll
2420 ↗(On Diff #185028)

Yes, I wasn't sure how that works

This revision was automatically updated to reflect the committed changes.