This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Fixed handling of imemdiate i1 literals
ClosedPublic

Authored by rampitec on May 10 2019, 4:43 PM.

Diff Detail

Event Timeline

rampitec created this revision.May 10 2019, 4:43 PM
arsenm added inline comments.May 12 2019, 3:20 PM
lib/Target/AMDGPU/SIInstrInfo.cpp
2526

I don’t understand where this is coming from. There should be no 1-bit immediates anywhere?

rampitec marked an inline comment as done.May 12 2019, 4:37 PM
rampitec added inline comments.
lib/Target/AMDGPU/SIInstrInfo.cpp
2526

Combiner sometimes produces "xor x, true", and even "add x, true". This is not the first time we hit it, we have even implemented lowering.

Why does this return false? A 1-bit immediate is either 0 or -1, both of which can be represented as inline constants everywhere.

arsenm added inline comments.May 13 2019, 7:40 AM
lib/Target/AMDGPU/SIInstrInfo.cpp
2526

I would expect this to be extended to the wavesize during selection

2526

I guess that wouldn't provide much benefit, but this should always be true?

FWIW, this fixes the regression I reported, without regressing any other tests I run.

rampitec marked an inline comment as done.May 13 2019, 8:07 AM

Why does this return false? A 1-bit immediate is either 0 or -1, both of which can be represented as inline constants everywhere.

Technically yes, but the query is about VOP literal, while i1 ends up in a SOP normally.

lib/Target/AMDGPU/SIInstrInfo.cpp
2526

Yes, these are extended, but we have the situation where predicate is checked before the extension.

While it technically can fit VOP literal I do not believe that would be a good idea. The bool operand shall go SOP instruction, so I preffered to return false. At the very least we will cancel unsuitable pattern matching earlier this way.

arsenm added inline comments.May 14 2019, 8:16 AM
lib/Target/AMDGPU/SIInstrInfo.cpp
2526

But SOP instructions have the same inline immediate values. This isn't a property of the immediate or instruction

arsenm added inline comments.May 14 2019, 8:23 AM
lib/Target/AMDGPU/SIInstrInfo.cpp
2526

OK, I see this is breaking not at the point I thought. In the bool case these still end up materialized with regular int64_t operands. This is only happening in the DAG patterns. Saying this is true should be fine, since it still works with the normal immediate folding heuristics.

rampitec updated this revision to Diff 199463.May 14 2019, 8:46 AM
rampitec marked 5 inline comments as done.

Changed return to true.

arsenm accepted this revision.May 14 2019, 9:06 AM

LGTM

test/CodeGen/AMDGPU/xor3-i1-const.ll
4

Grammar

10–20

This could probably be reduced a bit

This revision is now accepted and ready to land.May 14 2019, 9:06 AM
rampitec marked 2 inline comments as done.May 14 2019, 9:14 AM
rampitec added inline comments.
test/CodeGen/AMDGPU/xor3-i1-const.ll
10–20

I was unable to reduce it further. Combine conditions are peculiar.

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