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.
Paths
| Differential D17898
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
Event Timelinejvesely updated this object. Comment Actions What is emitting the illegal min/max? Isn't this already handled in DAGTypeLegalizer::PromoteIntegerResult? I added handling here a while ago Comment Actions
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 *** 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 Comment Actions What is emitting the illegal min/max? Isn't this already handled in DAGTypeLegalizer::PromoteIntegerResult? I added handling here a while ago
Min/max matching is done in SelectionDAGBuilder. You want -debug / -debug-only=isel Comment Actions
the culprit is SelectionDAGBuilder::visitSelect(const User &I), in lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2598. 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. Comment Actions
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
Comment Actions
Using i56 fails in LegalizeDAG.cpp:893
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. Comment ActionsTurns out signed version is not that different, so we can have unified version.
jvesely updated this object. jvesely marked 3 inline comments as done. Comment Actionsfactor out sub op selection
Comment Actions I can't comment on the test changes, but the code LGTM.
arsenm edited edge metadata. Comment ActionsLGTM
This revision is now accepted and ready to land.Jun 8 2016, 12:09 PM Closed by commit rL272272: SelectionDAG: Implement expansion of {S,U}MIN/MAX in integer legalization (authored by jvesely). · Explain WhyJun 9 2016, 9:10 AM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 49857 lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
lib/CodeGen/SelectionDAG/LegalizeTypes.h
test/CodeGen/AMDGPU/max.ll
test/CodeGen/AMDGPU/min.ll
test/CodeGen/AMDGPU/sminmax.ll
|
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.