Page MenuHomePhabricator

Teach TableGen Intrin Emitter to handle LLVMPointerType<llvm_any_ty>
ClosedPublic

Authored by erichkeane on Jun 18 2019, 11:30 AM.

Details

Summary

r363233 rewrote a bunch of the Intrin Emitter code, however the new
function to update the arg codes did not properly consider a pointer to
an any. This patch adds that logic.

Diff Detail

Event Timeline

erichkeane created this revision.Jun 18 2019, 11:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2019, 11:30 AM
Herald added a subscriber: wdng. · View Herald Transcript

Can someone take a look at this?
@sdesmalen in particular, I'd love it if you could take a look since you're the author of this code.

Unfortunately, I've never worked in the IntrinsicEmitter so I can't really comment on the correctness of the patch. I will make some inline comments on non-correctness things.

greened added inline comments.Jun 20 2019, 1:25 PM
llvm/test/TableGen/intrinsic-pointer-to-any.td
2

Could we have a comment here explaining what this is testing and what went wrong before this patch?

23

Do we need this and the below parameters for the test? AFAICT they are never passed.

26

Are the members here expected to exist by IntrinsicEmitter? If so, defaults could be set directly rather than using default arguments that are never passed. If not, we could just delete them.

erichkeane marked 5 inline comments as done.Jun 20 2019, 1:37 PM
erichkeane added inline comments.
llvm/test/TableGen/intrinsic-pointer-to-any.td
23

They're not needed, though I was trying to just emulate what the actual definition of Intrinsic was. I'll change it.

26

They all seem required. I can do that.

erichkeane marked 2 inline comments as done.

Can someone take a look at this?
@sdesmalen in particular, I'd love it if you could take a look since you're the author of this code.

Thanks, I definitely want to take a look but I’m currently traveling without access to my laptop, so I won’t be able to properly review it until Wednesday.

greened added inline comments.Jun 21 2019, 10:10 AM
llvm/utils/TableGen/IntrinsicEmitter.cpp
375

Since I don't know this code I can't say whether it's correct, but it seems odd to me that this iPTR case is needed to handle ptr-to-any when iPTRAny is a case right below. Does iPTRAny not mean "ptr-to-any?" At the very least a comment would be helpful explaining what is going on here and why we need a separate iPTR case to handle ptr-to-any.

erichkeane marked an inline comment as done.Jun 21 2019, 10:16 AM

Can someone take a look at this?
@sdesmalen in particular, I'd love it if you could take a look since you're the author of this code.

Thanks, I definitely want to take a look but I’m currently traveling without access to my laptop, so I won’t be able to properly review it until Wednesday.

Great, thanks!

llvm/utils/TableGen/IntrinsicEmitter.cpp
375

iPTRAny is actually "Any Pointer", not "Pointer to Any". I'm not terribly sure why it makes a difference, but it does.

hfinkel added inline comments.
llvm/utils/TableGen/IntrinsicEmitter.cpp
375

I believe that iPTR is a pointer in some specified address space (0, by default), and iPTRAny is a pointer in any address space.

hfinkel accepted this revision.Jun 25 2019, 4:10 PM

LGTM

This revision is now accepted and ready to land.Jun 25 2019, 4:10 PM
This revision was automatically updated to reflect the committed changes.

The patch already landed, but also adding my LGTM. It seems like a case that I missed in my previous patch, thanks for fixing!