[PATCH] D60887: [AsmPrinter] refactor to support %c w/ GlobalAddress'

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers marked an inline comment as done.
nickdesaulniers added inline comments.


================
Comment at: llvm/lib/Target/MSP430/MSP430AsmPrinter.cpp:101
+    O << '#';
+    PrintSymbolOperand(MO, O);
     return;
----------------
peter.smith wrote:
> 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.
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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60887/new/

https://reviews.llvm.org/D60887