This is an archive of the discontinued LLVM Phabricator instance.

[AsmPrinter] Add hook to override constant folding for printing.
Needs RevisionPublic

Authored by krzysz00 on Feb 7 2023, 9:50 AM.

Details

Summary

Instead of calling ConstantFoldConstant inside the asm printer, use
foldConstantForPrint(), which can be customized per-target.

(Also, fix printing null pointers of size > 64 bits.)

The motivating example for this change is the expression
ptr addrspace(7) addrcast(ptr null to ptr addrspace(7)) on AMDGPU once
address space 7 has been set to an accurate 128-bit length, which is a
change that will occur in a future commit.

The null pointer will be cast to a 128-bit "null descriptor", that is,
i128 0. However, 128-bit values do not fit into MCExpr, and cause
crashes when passed to emitIntValue(). Therefore, we add this constant
folding hook to allow the AMDGPU target to fix up the relevant address
space cast.

Diff Detail

Event Timeline

krzysz00 created this revision.Feb 7 2023, 9:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2023, 9:50 AM
krzysz00 requested review of this revision.Feb 7 2023, 9:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2023, 9:50 AM

(Feel free to suggest other reviewers)

arsenm added a comment.Feb 7 2023, 9:59 AM

Missing tests

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3454–3460

Don't understand why you can't unconditionally call emitZeros. Compatibility is not a concern with the codegen APIs

llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h
105

I don't understand this hook. Why is printing special? I'd rather extend with an MCBigConstantExpr or similar implementation rather than hack around this

llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp
178

Don't hardcode to generic, check the null pointer representation

arsenm added inline comments.Feb 7 2023, 10:01 AM
llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h
105

MCAPIntConstantExpr?

Re this being a hack, agreed. I'm open to doing something more principled like you suggested.

On the missing tests front, the problem is that to add a test, I'd need to swap the GCN data layout to include a p7:128:128:128:32 term, which, without a change like this one, causes the existing tests to crash.

Would it be more appropriate to make the data layout change first and then XFAIL (or comment out the relevant parts of) nulls.ll, to land that first, to land this despite a general lack of tests, or to put this and the data layout changes in one larger commit?

Re this being a hack, agreed. I'm open to doing something more principled like you suggested.

On the missing tests front, the problem is that to add a test, I'd need to swap the GCN data layout to include a p7:128:128:128:32 term, which, without a change like this one, causes the existing tests to crash.

Would it be more appropriate to make the data layout change first and then XFAIL (or comment out the relevant parts of) nulls.ll, to land that first, to land this despite a general lack of tests, or to put this and the data layout changes in one larger commit?

Why would they crash? Just make the datalayout change first, there's little reason to put that one off

The crash is because of emitIntValue() having an assert(Size <= 8) in it

A bit strange but I think it is fine to have the hook if AMDGPU really needs it.

The crash is because of emitIntValue() having an assert(Size <= 8) in it

I mean why do we have emitIntValue at all. Why isn't emitZeros always used?

@MaskRay Yeah, this is strange, and I'm with @arsenm on needing to find a less ugly solution to this problem.

Re emitZeros: I suspect there may be some reason to use, say, .quad 0 or .long 0 over .zeroes N, and that difference might actually matter when you're dealing with debug info or something? I'm not sure, but I didn't want to upset the structures.

For context, the call chain that hits the bug is as follows

  1. The problematic expression (addrspace cast ptr null to addrspace(7)) hits emitGlobalConstantImpl
  2. There, it's not recoginized as any of the cases that emitGlobalConstantImpl knows how to handle by itself, so it calls lowerConstant to produce an MCExpr from said expression
  3. We step in and do the "adddrspace cast of null pointers" handling, which, in this case, would just return a 0, and we correctly tag that 0 with a size of 16 bytes/128 bits.
  4. Said 0, being an MCExpr, gets sent to emitValue(), which sends it to emitIntValue().

4b. That MCExpr holds a uint64_t, not an APInt, so none of the MCExpr code is prepared to deal with values over 64 bits.

  1. emitIntValue() sensibly asserts that the value it's being asked to emit is 8 bytes or less, which we just broke

My hack was to provide a pathway for targets to send a value back to the start of emitGlobalConstantImpl(), but that doesn't really solve the problem.

nikic resigned from this revision.Feb 14 2023, 1:43 AM

(I don't have the necessary MC / AsmPrinter context here)

I think adding a big-int MCExpr would not be difficult and would avoid the need for this hack to specially handle 0

Having done some looking, I don't think that it's trivial to add bigint support to MCExpr, since that would mean that the MC emitting infrastructure would need to know the target endianness, which it doesn't look like it does.

And there's a lot of functions on MCExpr that return [u]int64_t, and we'd either need those to fail on big ints or we'd need to rework them to use APInts or something.

arsenm requested changes to this revision.May 23 2023, 3:26 AM

Having done some looking, I don't think that it's trivial to add bigint support to MCExpr, since that would mean that the MC emitting infrastructure would need to know the target endianness, which it doesn't look like it does.

This doesn't make any sense, of course it has to know the endianness? The endianness is the most global of flags in the TM and DataLayout

This revision now requires changes to proceed.May 23 2023, 3:26 AM