Page MenuHomePhabricator

Implement expansion of {s,u}{min,max} in integer legalization
ClosedPublic

Authored by jvesely on Mar 4 2016, 2:51 PM.

Details

Summary

SelectionDAGBuilder::visitSelect(const User &I) uses min/max for illegal types and expects type legalization to handle it.

Fixes {u,}long_{min,max,clamp} opencl piglit regressions on r600.

Diff Detail

Repository
rL LLVM

Event Timeline

jvesely updated this revision to Diff 49857.Mar 4 2016, 2:51 PM
jvesely retitled this revision from to Implement expansion of {s,u}{min,max}.
jvesely updated this object.
jvesely added reviewers: arsenm, tstellarAMD.
jvesely set the repository for this revision to rL LLVM.
jvesely added a subscriber: llvm-commits.
arsenm edited edge metadata.Mar 4 2016, 3:35 PM

What is emitting the illegal min/max?

Isn't this already handled in DAGTypeLegalizer::PromoteIntegerResult? I added handling here a while ago

What is emitting the illegal min/max?

Isn't this already handled in DAGTypeLegalizer::PromoteIntegerResult? I added handling here a while ago

I don't think this ever went through promotion pass since the types are i64 to begin with. Finding and fixing the place is probably better than undoing it in type legalization, but I had trouble finding the place
with dumps enabled, the last thing that I see before assertion failure is:

*** IR Dump After Module Verifier ***
; Function Attrs: norecurse nounwind uwtable
define void @test(i64* nocapture %out, i64 %a, i64 %b) #0 {
entry:
  %cmp = icmp sgt i64 %a, %b
  %cond = select i1 %cmp, i64 %a, i64 %b
  store i64 %cond, i64* %out, align 8, !tbaa !7
  ret void
}
ExpandIntegerResult #0: t16: i64 = smax t12, t13

Do not know how to expand the result of this operator!

the next pass is: AMDGPU DAG->DAG Pattern Instruction Selection
I'm not sure if llvm dump is of much use as afaik these ops don't show up there (which I assume means that they are not used).
the added test cases trigger the problem for EG.

arsenm added a comment.Mar 9 2016, 3:03 PM

What is emitting the illegal min/max?

Isn't this already handled in DAGTypeLegalizer::PromoteIntegerResult? I added handling here a while ago

What is emitting the illegal min/max?

Isn't this already handled in DAGTypeLegalizer::PromoteIntegerResult? I added handling here a while ago

I don't think this ever went through promotion pass since the types are i64 to begin with. Finding and fixing the place is probably better than undoing it in type legalization, but I had trouble finding the place
with dumps enabled, the last thing that I see before assertion failure is:

*** IR Dump After Module Verifier ***
; Function Attrs: norecurse nounwind uwtable
define void @test(i64* nocapture %out, i64 %a, i64 %b) #0 {
entry:
  %cmp = icmp sgt i64 %a, %b
  %cond = select i1 %cmp, i64 %a, i64 %b
  store i64 %cond, i64* %out, align 8, !tbaa !7
  ret void
}
ExpandIntegerResult #0: t16: i64 = smax t12, t13

Do not know how to expand the result of this operator!

the next pass is: AMDGPU DAG->DAG Pattern Instruction Selection
I'm not sure if llvm dump is of much use as afaik these ops don't show up there (which I assume means that they are not used).
the added test cases trigger the problem for EG.

Min/max matching is done in SelectionDAGBuilder. You want -debug / -debug-only=isel

What is emitting the illegal min/max?

the culprit is SelectionDAGBuilder::visitSelect(const User &I), in lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2598.
it transforms specifc single use comparisons into min/max.

It does not check legality of min/max operations. even for the operations it does check, it considers type after type legalization (so the check would pass on r600). The comment on line 2610 indicates that it intentionally relies on type legalizer handling illegal ops/types.

arsenm added a comment.Mar 9 2016, 4:46 PM

What is emitting the illegal min/max?

the culprit is SelectionDAGBuilder::visitSelect(const User &I), in lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2598.

it transforms specifc single use comparisons into min/max.

It does not check legality of min/max operations. even for the operations it does check, it considers type after type legalization (so the check would pass on r600). The comment on line 2610 indicates that it intentionally relies on type legalizer handling illegal ops/types.

OK, this is what I thought I did but it looks like I only did the promotion of integer types handling. I think new tests should be added for a weird sized integer between 32 and 64 that needs to be legalized to i64

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
1692–1694 ↗(On Diff #49857)

Why is the code so different for each of these? I would expect the same function with the CondCode as an operand

OK, this is what I thought I did but it looks like I only did the promotion of integer types handling. I think new tests should be added for a weird sized integer between 32 and 64 that needs to be legalized to i64

Using i56 fails in LegalizeDAG.cpp:893
Assertion `TLI.isTypeLegal(StVT) && "Do not know how to expand this store!"' failed.

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
1692–1694 ↗(On Diff #49857)

signed version can be used for both as it just reverts back to compare+select.

unsigned version tries to be smarter and recursively applies min/max to low part if high parts are equal (I think this was the intention behind the comment in visitSelect, but the signed version is rather messy).
I can switch the Hi select to another min/max to reduce dependencies.

jvesely updated this revision to Diff 50338.Mar 10 2016, 1:01 PM
jvesely retitled this revision from Implement expansion of {s,u}{min,max} to Implement expansion of {s,u}{min,max} in integer legalization.
jvesely updated this object.
jvesely edited edge metadata.

Turns out signed version is not that different, so we can have unified version.

arsenm added inline comments.Mar 21 2016, 8:37 AM
lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
1679–1685 ↗(On Diff #50338)

I would prefer moving this to a static function with a switch and returns of the enums

jvesely updated this revision to Diff 51606.Mar 24 2016, 2:54 PM
jvesely updated this object.
jvesely marked 3 inline comments as done.

factor out sub op selection

gentle ping.
any comments on the latest diff?

arsenm added inline comments.Apr 12 2016, 8:56 AM
lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
1671–1673 ↗(On Diff #51606)

Capitalization wrong: getMinMaxOps, brace should be on same line.

Should return the CondCode instead of assign and break which

jvesely updated this revision to Diff 53580.Apr 13 2016, 9:41 AM

return cond code as return value. change capitalization

jvesely marked an inline comment as done.Apr 28 2016, 6:26 PM

gentle ping

ping.

not sure what ping freq is customary, hope weekly is not too much.

ab added a subscriber: ab.Jun 1 2016, 2:15 PM

I can't comment on the test changes, but the code LGTM.

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
1671 ↗(On Diff #53580)

Maybe go even further and return a std::pair, and std::tie it into LoOpc/CondC ?

Also, maybe make it more explicit by calling it, say, "getExpandedMinMaxOps", and/or adding a comment.

1684 ↗(On Diff #53580)

dl -> DL

jvesely updated this revision to Diff 59287.Jun 1 2016, 3:08 PM

c++ fancify

jvesely marked 2 inline comments as done.Jun 1 2016, 3:10 PM
arsenm accepted this revision.Jun 8 2016, 12:09 PM
arsenm edited edge metadata.

LGTM

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
1675 ↗(On Diff #59287)

return on separate line

This revision is now accepted and ready to land.Jun 8 2016, 12:09 PM
This revision was automatically updated to reflect the committed changes.