Page MenuHomePhabricator

[IR] Prefer scalar type for struct indexes in GEP constant expressions.
ClosedPublic

Authored by efriedma on Jun 17 2020, 4:47 PM.

Details

Summary

Using a scalar constant instead of a vector constant has two advantages: one, it's simpler to understand, and two, it doesn't require complex pattern matching to work with scalable vectors.

Also includes a small fix to DataLayout to allow the scalable vector testcase to work correctly.

Diff Detail

Event Timeline

efriedma created this revision.Jun 17 2020, 4:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2020, 4:47 PM
ctetreau added inline comments.Jun 17 2020, 5:24 PM
llvm/lib/IR/Constants.cpp
2178–2180

can this use auto?

2178–2180

what is the change to iterators from indices buying us?

llvm/test/Analysis/ConstantFolding/vectorgep-crash.ll
27

the old thing should still work right? And presumably it's testing some crash that happened, perhaps we should leave it alone?

efriedma marked 2 inline comments as done.Jun 17 2020, 6:24 PM
efriedma added inline comments.
llvm/lib/IR/Constants.cpp
2178–2180

I think this can use auto, yes.

The point of using the iterators is that they provide the isStruct() and isSequential() methods.

llvm/test/Analysis/ConstantFolding/vectorgep-crash.ll
27

I'm only changing the CHECK line, not the input IR

ctetreau added inline comments.Jun 18 2020, 10:46 AM
llvm/lib/IR/Constants.cpp
2178–2180

Could these not be replaced by isa checks? isSequential seems especially useless since it checks that the thing is a Type *

efriedma marked an inline comment as done.Jun 18 2020, 11:39 AM
efriedma added inline comments.
llvm/lib/IR/Constants.cpp
2178–2180

At a high level, generic_gep_type_iterator takes a GEP base type and some indexes, and steps through the indexed types. The computed types can be queried with isStruct()/isSequential()/etc. This is information which we can't get directly from the indexes themselves.

At a low level, I think you're misreading the implementation of generic_gep_type_iterator; look at the documentation for PointerUnion.

sdesmalen added inline comments.Jun 19 2020, 8:15 AM
llvm/lib/IR/Constants.cpp
2190–2191

What is the reason for not always using a scalar index value?

efriedma marked an inline comment as done.Jun 19 2020, 11:03 AM
efriedma added inline comments.
llvm/lib/IR/Constants.cpp
2190–2191

In my thinking, there are two reasons:

  1. I wanted to be conservative; arbitrary constant expressions are legal in this position anyway, so the harm from code failing to understand a vector splat is much smaller.
  2. We don't want to accidentally change the result type of a GEP if every operand is a splat.
sdesmalen added inline comments.Jun 19 2020, 2:22 PM
llvm/lib/IR/Constants.cpp
2190–2191

Okay so if I understand that correctly, we can do this change safely for struct types without risking to change the result type. because in e.g.:

%gep = getelementptr %struct, %struct* null, i32 0, <4 x i32> <i32 1, i32 1, i32 1, i32 1>

the i32 0 will always be expanded into a vector:

%gep = getelementptr %struct, %struct* null, <4 x i32> zeroinitializer, i32 1

And so there is always at least one index that will remain a vector (splat).

efriedma marked an inline comment as done.Jun 19 2020, 4:02 PM
efriedma added inline comments.
llvm/lib/IR/Constants.cpp
2190–2191

Yes, exactly.

This revision is now accepted and ready to land.Jun 22 2020, 12:07 AM

I guess my only real remaining issue is to use auto for the type of the iterators. Otherwise this looks good to me.

This revision was automatically updated to reflect the committed changes.