Page MenuHomePhabricator

[SelectionDAG][X86] Explicitly store the scale in the gather/scatter ISD nodes
ClosedPublic

Authored by craig.topper on Nov 14 2017, 3:42 PM.

Details

Summary

Currently we infer the scale at isel time by analyzing whether the base is a constant 0 or not. If it is we assume scale is 1, else we take it from the element size of the pass thru or stored value. This seems a little weird and I think it makes more sense to make it explicit in the DAG rather than doing tricky things in the backend.

Most of this patch is just making sure we copy the scale around everywhere.

The test changes are interesting because I believe the current codegen is actually a bug. We seem to be promote v2i32 gathers to v2i64 and double the scale. But really the address calculation shouldn't have been affected. This is a consquence of getting the scale from the data size. With it stored explicitly it comes from the original GEP. I don't think we should be promoting v2i32 gathers to v2i64, the IR we started with only said it was legal to read 32 bits at each index. If one of the elements is at the end of an array near a page boundary the extra data read might cause a fault. I'll look into a fixing this as a separate patch.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Nov 14 2017, 3:42 PM
delena added inline comments.Nov 16 2017, 9:40 PM
test/CodeGen/X86/masked_gather_scatter.ll
1275 ↗(On Diff #122931)

gatherQQ is a bug, we should read dwords. And scale 4 does not help. I've fixed this bug in some cases, probably not in all.
As far as I remember, the community voted against the scale - they said that I'm trying to push X86 specific things into the common nodes.
So I waited until instruction selection in push the scale. And I created X86MaskedGather to cover all other things, mainly <2 x i32> types.

Yes I'm going to fix the QQ issue as well.

The scale would have been part of the handling of the last index of the GEP if we hadn't split it. So from that perspective its not X86 specific. Its part of the GEP calculation. Do you recall who objected? Maybe we can revisit.

Alternatively, I wonder if it would be better to have a target hook to directly turn gather into a target specific ISD node. The generic type legalization code does have some trouble with the index argument as it is. We're currently doing various custom things to avoid certain cases.

@hfinkel do you have any opinions on this?

reames added a comment.Jan 8 2018, 4:12 PM

This looks like the right approach to me. I think the previously expressed concern was about making the IR representation overly specific to what current hardware supports. This looks largely orthogonal to me and much less confusing as well.

This looks like the right approach to me. I think the previously expressed concern was about making the IR representation overly specific to what current hardware supports. This looks largely orthogonal to me and much less confusing as well.

I agree.

RKSimon added inline comments.Jan 9 2018, 3:50 AM
lib/CodeGen/SelectionDAG/SelectionDAG.cpp
6234 ↗(On Diff #122931)

Should we assert that Scale op is a positive (pow2?) constant?

Rebase and add the asserts Simon requested

zvi added inline comments.Jan 10 2018, 4:32 AM
lib/Target/X86/X86ISelDAGToDAG.cpp
1533 ↗(On Diff #129151)

Remove the line above?

Remove unneeded line that snuck in during rebase.

zvi added a comment.Jan 10 2018, 9:50 AM

There are some occurrences of calls to getMaskedGather in DAGCombine.cpp which i do not see being addressed by this patch. I guess they are not being covered by tests?

Fix the calls in DAG combine and add test cases for it.

delena accepted this revision.Jan 10 2018, 11:06 AM
This revision is now accepted and ready to land.Jan 10 2018, 11:06 AM
This revision was automatically updated to reflect the committed changes.