This is an archive of the discontinued LLVM Phabricator instance.

[AsmPrinter] refactor to support %c w/ GlobalAddress'
ClosedPublic

Authored by nickdesaulniers on Apr 18 2019, 1:36 PM.

Details

Summary

Targets like ARM, MSP430, PPC, and SystemZ have complex behavior when
printing the address of a MachineOperand::MO_GlobalAddress. Move that
handling into a new overriden method in each base class. A virtual
method was added to the base class for handling the generic case.

Refactors a few subclasses to support the target independent %a, %c, and
%n.

The patch also contains small cleanups for AVRAsmPrinter and
SystemZAsmPrinter.

It seems that NVPTXTargetLowering is possibly missing some logic to
transform GlobalAddressSDNodes for
TargetLowering::LowerAsmOperandForConstraint to handle with "i" extended
inline assembly asm constraints.

Fixes:

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2019, 1:36 PM
nickdesaulniers marked an inline comment as done.Apr 18 2019, 1:49 PM

When I added test cases for RISCV and AVR, I got the following error message from llvm-lit that I don't understand:

UNSUPPORTED: LLVM :: CodeGen/RISCV/inlineasm-output-template.ll (1 of 1)
UNSUPPORTED: LLVM :: CodeGen/AVR/inlineasm-output-template.ll (1 of 1)

I'm not setting -DLLVM_TARGETS_TO_BUILD cmake variable, so I guess these backends are defaulted off or something?

llvm/test/CodeGen/NVPTX/inlineasm-output-template.ll
12

I'm not particularly happy about this, but didn't have enough background with SelectionDAG to quickly understand the issue. Please let me know if I should drop this added but commented out test case from this patch.

so I guess these backends are defaulted off or something?

@srhines identified that AVR and RISCV are not in LLVM_ALL_TARGETS in llvm/CMakeLists.txt. Should I send a patch to turn them on? (assuming the tests aren't red).

so I guess these backends are defaulted off or something?

@srhines identified that AVR and RISCV are not in LLVM_ALL_TARGETS in llvm/CMakeLists.txt. Should I send a patch to turn them on? (assuming the tests aren't red).

I think the AVR and RISCV backends are, or at least were, in "experimental" state so weren't included in LLVM_ALL_TARGETS. I thought that RISCV at least had been promoted out of experimental but I guess the backend owner (ASB I think) hasn't done that yet.

I've checked the patch and it looks good from the Arm/AArch64 perspective. Only thing I spotted was a change to the MSP340 that I didn't understand, but that could be intentional.

llvm/lib/Target/MSP430/MSP430AsmPrinter.cpp
101

This looks to have taken out the case when isMemOp is true (the removed code could output &). I don't know enough about MSP430 to know whether this is the right thing to do though.

nickdesaulniers marked an inline comment as done.Apr 24 2019, 4:16 PM
nickdesaulniers added inline comments.
llvm/lib/Target/MSP430/MSP430AsmPrinter.cpp
101

The conditional in the initialization of isMemOp and the condition I removed here are quite tricky and require careful attention.

The unconditional printing of '#' comes from the observation in the previous version of the code that:

  1. there is no such modifier "mem" ever
  2. thus strcmp will always be non-zero
  3. thus !strcmp will always be 0
  4. thus Modifier && !strcmp will always be false
  5. thus isMemOp will always be false
  6. thus (isMemOp ? '&' : '#') will always evaluate to '#'

The next transform is based on the above AND the observations:

  1. the only modifier is "nohash"
  2. thus if there is no Modifier, print '#'
  3. or if the only possible modifier is the only possible value, print '#'
  4. thus what we have is an unconditional print of '#'

But I will admit this was tricky when first looking at it and would appreciate you triple checking as I may have made a mistake in the above logic. It's definitely the most curious part of this patch for sure. Also, sorry if I misunderstood your point. Thanks for your review!

nickdesaulniers marked an inline comment as done.Apr 24 2019, 4:20 PM
nickdesaulniers added inline comments.
llvm/lib/Target/MSP430/MSP430AsmPrinter.cpp
101

Also, I had previously tackled this in https://reviews.llvm.org/D60738, but abandoned it when I started the overall fix, as I needed it one way or another. Happy to revive that there, land it first, then rebase this if you prefer?

peter.smith added inline comments.Apr 25 2019, 2:18 AM
llvm/lib/Target/MSP430/MSP430AsmPrinter.cpp
96

This comment looks to be out of date as & will never be printed.

101

I agree with the first part about "mem". I'm not sure about the second as it is strcmp and not !strcmp. With printOperand(MI, OpNum+1, O, "nohash"); we would have
Modifier = "nohash".
In this case

if (!Modifier || strcmp(Modifier, "nohash"))
  O << (isMemOp ? '&' : '#');

Would end up with the condition failing as strcmp("nohash", "nohash") == 0 and !Modifier also being false so we would not output a '#' in this case. So I think it should be:

if (!Modifier || strcmp(Modifier, "nohash"))
      O << '#';

As in line 86.

Hope I have that right, it has been a long time since I've read strcmp/!strcmp.

Also, I had previously tackled this in https://reviews.llvm.org/D60738, but abandoned it when I started the overall fix, as I needed it one way or another. Happy to revive that there, land it first, then rebase this if you prefer?

I don't have a strong preference. This was the only part of the patch that I saw that made significant changes that I didn't know where they came from so it looked like it could have been an oversight.

  • - fix MSP430AsmPrinter '#', adjust comment
nickdesaulniers marked 4 inline comments as done.Apr 25 2019, 10:08 AM
nickdesaulniers added inline comments.
llvm/lib/Target/MSP430/MSP430AsmPrinter.cpp
101

Great catch!

void accepted this revision.Apr 25 2019, 12:59 PM
void added a subscriber: void.

This looks like a nice cleanup. Accepting unless Peter has any objections.

This revision is now accepted and ready to land.Apr 25 2019, 12:59 PM

This looks like a nice cleanup. Accepting unless Peter has any objections.

I don't have any objections.

This revision was automatically updated to reflect the committed changes.
nickdesaulniers marked an inline comment as done.