This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] hazard recognizer for fp atomic to s_denorm_mode
ClosedPublic

Authored by rampitec on Jun 20 2019, 12:29 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

rampitec created this revision.Jun 20 2019, 12:29 PM
arsenm added inline comments.Jun 20 2019, 12:43 PM
lib/Target/AMDGPU/GCNHazardRecognizer.cpp
148 ↗(On Diff #205877)

Typo Denrom

1156–1157 ↗(On Diff #205877)

Assuming false for memoperands_empty is wrong

1157–1163 ↗(On Diff #205877)

This whole approach to identifying the atomic opcodes doesn't really work. You can't rely on any of the information in the memoperand, especially the IR pointee type. We do have an SIInstrFlags::maybeAtomic, but I think you need to check the opcode for if it's an FP one

1161–1163 ↗(On Diff #205877)

Can't use the pointee type

test/CodeGen/AMDGPU/fp-atomic-to-s_denormmode.mir
38 ↗(On Diff #205877)

Should test all the FP atomic opcodes

rampitec updated this revision to Diff 205882.Jun 20 2019, 12:53 PM
rampitec marked 3 inline comments as done.
rampitec added inline comments.
lib/Target/AMDGPU/GCNHazardRecognizer.cpp
1157–1163 ↗(On Diff #205877)

maybeAtomic will catch too much, including atomic loads and stores. I can use "AMDGPU::getAtomicRetOp(I->getOpcode()) != -1" to catch it.
However listing all possible opcodes is error prone and not future proof, especially given that many of them are just commented out currently. If you believe this value is unreliable I will have to use another bit in the MI flags. Does it sound good?

arsenm added inline comments.Jun 20 2019, 1:07 PM
lib/Target/AMDGPU/GCNHazardRecognizer.cpp
1157–1163 ↗(On Diff #205877)

Another bit would be fine

rampitec updated this revision to Diff 205910.Jun 20 2019, 3:02 PM
rampitec marked 4 inline comments as done.

LGTM assuming the DS atomics really aren't impacted

lib/Target/AMDGPU/BUFInstructions.td
696 ↗(On Diff #205910)

I didn't know you could omit the braces here

arsenm accepted this revision.Jun 21 2019, 6:31 AM
This revision is now accepted and ready to land.Jun 21 2019, 6:31 AM
rampitec marked an inline comment as done.Jun 21 2019, 9:01 AM

LGTM assuming the DS atomics really aren't impacted

According to the ticket that is only vmem.

lib/Target/AMDGPU/BUFInstructions.td
696 ↗(On Diff #205910)

You do not need braces to cover a single def.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2019, 9:27 AM