Page MenuHomePhabricator

[AsmPrinter] handles addrspacecast in lowerConstant
AbandonedPublic

Authored by jingyue on Apr 16 2015, 4:24 PM.

Details

Diff Detail

Event Timeline

jingyue updated this revision to Diff 23888.Apr 16 2015, 4:24 PM
jingyue retitled this revision from to [AsmPrinter] handles addrspacecast in lowerConstant.
jingyue updated this object.
jingyue edited the test plan for this revision. (Show Details)
jingyue added reviewers: eliben, jholewinski.
jingyue added a subscriber: Unknown Object (MLST).
jholewinski added inline comments.Apr 16 2015, 4:31 PM
test/CodeGen/NVPTX/addrspacecast-gvar.ll
6

Shouldn't this be {0, generic(g)+8} ?

I think you're right. LMTAL why I'm missing the generic.

This issue turns out more difficult to fix than I expected. With the test case in my patch, LLVM runs into NVPTXAsmPrinter.h:163 and prints out an MCExpr that represents a GEP of an addrspacecast (i32* getelementptr (i32, i32* addrspacecast (i32 addrspace(1)* @g to i32*), i32 2)). Even if we fix lowerConstant to translate addrspacecast into an MCUnaryExpr, printing an MCExpr is not customized yet so we are still unable to print the MCUnaryExpr with generic(.

Can we customize the printing of MCExpr?

An alternative is to have NVPTXFavorNonGenericAddrSpaces to canonicalize ConstantExpr gep(addrspacecast( to addrspacecast(gep( (this pass currently canonicalizes only Instructions and ConstantExprs that are reachable from load or store), and then NVPTXAsmPrinter can just handle shallower patterns.

jholewinski edited edge metadata.Apr 20 2015, 5:35 AM

Handling this aspect of PTX has always been a PITA. Internally, we customize lowerConstant to handle this case and create an MCExpr subclass (NVPTXGenericMCSymbolRefExpr) that handles the printing of "generic(<symbolref>)". Something like this will be needed upstream. Our internal version is a bit of a hack, so I'll want to come up with something cleaner for upstream. If you don't have time to tackle that, I'll try to put something together soon.

It would be awesome if you can upstream the customized lowerConstant. Thanks in advance!

At the same time, I'd like to try the canonicalization idea I mentioned. I feel this path will be fast and can work around our internal urgent issues. After your upstreaming is done, I'll (partially) revert my canonicalization. How does that sound to you?

Sure, go for it! It may be a few days before I get around to looking at this.

I committed a fix for this in r236000

jingyue abandoned this revision.Apr 30 2015, 5:24 PM

Great! Thanks very much, Justin.