[PATCH] D60887: [AsmPrinter] refactor to support %c w/ GlobalAddress'
Peter Smith via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 25 02:23:40 PDT 2019
peter.smith added inline comments.
================
Comment at: llvm/lib/Target/MSP430/MSP430AsmPrinter.cpp:96
// register base, we should not emit any prefix symbol here, e.g.
// mov.w &foo, r1
// vs
----------------
This comment looks to be out of date as & will never be printed.
================
Comment at: llvm/lib/Target/MSP430/MSP430AsmPrinter.cpp:101
+ O << '#';
+ PrintSymbolOperand(MO, O);
return;
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > 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!
> 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 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60887/new/
https://reviews.llvm.org/D60887
More information about the llvm-commits
mailing list