[PATCH] D66329: [PowerPC] [Peephole] fold frame offset by using index form to save add.

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2703
+  // TODO: sync the logic between instrHasImmForm() and ImmToIdxMap.
+  if (!HasImmForm)
+    return false;
----------------
stefanp wrote:
> Since `HasImmForm` is only used here you can put it directly into the if statement.
> 
> However, I'm a little confused as to why you have to call `instrHasImmForm` and then check the return. It seems like you start with the immediate form and then move on to get the X-Form for it and then you ask to see if there is an immediate. Is it to fill in the `III` object with the correct flags?
> 
> If that is the case we need to have another function to set the flags correctly for an arbitrary immediate opcode. That, however is out of the scope of this patch.
Yes, the flags in `III` is what this patch needs later. Because the opcode handling in `instrHasImmForm` and `ImmToIdxMap` is not the same, so I do a little redundant checking for load/store opcodes to avoid functionality issue.

We already have a plan to redesign the mapping representation between index form and displacement form by using `InstrMapping` in td files. After that I think we can avoid such code.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2706
+
+  if (!III.IsSummingOperands)
+    return false;
----------------
amyk wrote:
> This may be a silly question, but I am curious as to what this line means.
I think it means "Is the instruction summing the operand". In load/store instruction, it means we get the EA by summing its operands. For example, for instruction `grA = ld Imm, grB`, the EA is calculated by `Imm + grB`

Here we want to put `OffsetImm` of load/store to ADDI immediate operand, so we must ensure load/store is IsSummingOperands?


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

https://reviews.llvm.org/D66329