Page MenuHomePhabricator

getMangledTypeStr: clarify how it mangles types, and add tests
ClosedPublic

Authored by artagnon on Jan 10 2015, 4:23 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

artagnon updated this revision to Diff 17979.Jan 10 2015, 4:23 PM
artagnon retitled this revision from to getMangledTypeStr: add support for vectors.
artagnon updated this object.
artagnon edited the test plan for this revision. (Show Details)
artagnon added reviewers: chandlerc, void, rafael.
artagnon added a subscriber: Unknown Object (MLST).
sanjoy added a subscriber: sanjoy.Jan 10 2015, 4:37 PM

Two nits inline. Also, do we need a test case here?

lib/IR/Function.cpp
451 ↗(On Diff #17979)

Modify this comment to reflect that we handle vectors too.

466 ↗(On Diff #17979)

I think this will be better as VectorType* VTyp = dyn_cast<VectorType>(Ty)

artagnon updated this revision to Diff 17995.Jan 11 2015, 8:18 PM

After some investigation, I have concluded that this change is
completely incorrect. For integers, floats and vectors, it is supposed
to fall back to the getEVTString() which may then return "i", "v",
"f" prefixes in the common case.

I have instead focused on updating the comment with this information,
and writing a set of tests that people can use as a guide for coming
up with the correct mangling string.

artagnon retitled this revision from getMangledTypeStr: add support for vectors to getMangledTypeStr: clarify how it mangles types, and add tests.Jan 11 2015, 8:20 PM
artagnon updated this object.
reames added a subscriber: reames.Jan 12 2015, 10:22 AM

Comments inline. I like the general direction of the revised patch.

lib/IR/Function.cpp
450 ↗(On Diff #17995)

Please separate line breaking differences into their own patch. It's hard to tell what actually changed in this comment.

459 ↗(On Diff #17995)

x88mmx isn't what actually appears in your test? Why the difference? (Clarify in comment please.)

FYI - I expect you'll run into some problems actually lowering vectors gc.relocates. It's not a case we've explored.

test/CodeGen/Generic/overloaded-intrinsic-name.ll
1 ↗(On Diff #17995)

This is probably better as an opt -verify test. Not sure CodeGen is the right place for it either.

Alternatively, you could go the other direction and adapt this to be a gc lowering specific test. This would involve checking the assembly output, but might be reasonable tor exercising the vector path it seems like you're interested in.

4 ↗(On Diff #17995)

You might want to clarify that the use of gc.* is arbitrary and that these are no GC tests. It's mostly clear, but could be a bit more explicit.

7 ↗(On Diff #17995)

Given that these don't have check statements, how do you judge correctness of the tests?

artagnon added inline comments.Jan 12 2015, 1:32 PM
lib/IR/Function.cpp
459 ↗(On Diff #17995)

x86mmx is an x86-specific type that's actually a 64-bit vector. There are already tests specifically for mmx in the same directory; I just want to document common name manglings in one place.

Yeah, I'll look into gc.relocate and vectors after this.

test/CodeGen/Generic/overloaded-intrinsic-name.ll
1 ↗(On Diff #17995)

opt -verify -S it is then. Why not codegen? I can see plenty of similar code in the same directory.

I'll add to statepoint-call-lowering in subsequent patches.

7 ↗(On Diff #17995)

We just want to ensure that it runs fine. Specifically checking name mangling against the types of arguments is not something I'm aiming for in this patch.

artagnon updated this revision to Diff 18043.Jan 12 2015, 1:34 PM

Fix nits. Mostly the same.

reames added inline comments.Jan 12 2015, 4:24 PM
test/CodeGen/Generic/overloaded-intrinsic-name.ll
1 ↗(On Diff #17995)

I have to admit, I didn't look at the nearby tests. If they had a particular style for testing the same thing, feel free to keep that.

7 ↗(On Diff #17995)

Fine. Add a comment to that effect please? Top of the file is fine.

artagnon updated this revision to Diff 18092.Jan 13 2015, 9:41 AM

From reames review: Add a comment on top of the test file stating its
purpose.

This revision was automatically updated to reflect the committed changes.