Page MenuHomePhabricator

Remove CompositeType class
ClosedPublic

Authored by efriedma on Mar 4 2020, 6:46 PM.

Details

Summary

The existence of the class is more confusing than helpful, I think; the commonality is mostly just "GEP is legal", which can be queried using APIs on GetElementPtrInst.

Diff Detail

Event Timeline

efriedma created this revision.Mar 4 2020, 6:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2020, 6:46 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
huihuiz added a subscriber: huihuiz.Mar 5 2020, 1:32 PM
efriedma edited reviewers, added: greened; removed: dmgreen.

Thanks for working on this @efriedma!

llvm/include/llvm/IR/Instructions.h
1020

Please add some comments/documentation for these methods.

llvm/lib/IR/Instructions.cpp
1661–1662

Is there a reason you didn't just call this getTypeAtIndex ?

1667

Maybe I'm misunderstanding this method, but checking that the indexed type is of type integer, does that exclude the case:

getelementptr float, float* %ptr, i64 0

?

1681

nit: can this use const IndexTy &?

efriedma marked 3 inline comments as done.Mar 17 2020, 10:01 AM
efriedma added inline comments.
llvm/lib/IR/Instructions.cpp
1661–1662

I guess I could call it that; didn't really think much about the name. (I originally thought of it as splitting out each "step" of getIndexedType into a separate method.)

1667

The point of the isIntOrIntVectorTy() check is to exclude something silly like a float index into an array. It has nothing to do with the type of the pointer.

1681

Yes, but it wouldn't really matter; IndexTy is cheap to copy.

sdesmalen added inline comments.Mar 17 2020, 10:18 AM
llvm/lib/IR/Instructions.cpp
1667

Right, it's calling isIntOrIntVectorTy on the Idx, not Ty, I misread that.

efriedma updated this revision to Diff 250854.Mar 17 2020, 12:16 PM

Address review comments.

sdesmalen accepted this revision.Mar 17 2020, 2:34 PM

Thanks for the changes, the patch looks good to me!

This revision is now accepted and ready to land.Mar 17 2020, 2:34 PM
efriedma updated this revision to Diff 250909.Mar 17 2020, 2:38 PM

Add one missing change in clang unittests

Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2020, 2:38 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
This revision was automatically updated to reflect the committed changes.