[PATCH] D27984: [ARM] [RFC] Use AddDefaultCC/Pred where possible. NFC

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.

I do think that using addDefaultPred/CC makes the code more readable compared to using .addreg(0) or .addImm((int64_t)ARMCC::AL).addReg(0), especially for newcomers to the ARM backend: it makes the BuildMI lines look that little bit more in line with ARM assembly syntax. In other words, it is easier to guess what "AddDefaultPred" or "AddDefaultCC" mean than ".addreg(0)", when the ARM assembly syntax for the corresponding instruction doesn't have that extra operand...

I don't have a strong preference over using addDefaultPred/CC or the other alternative discussed syntaxes like MIB.addSomeOperand().addSomeOtherOperand().addDefaultPred(), but I do think that the existing addDefaultPred/CC syntax may keep the BuildMI lines a little bit more similar to the ARM assembly syntax for the instruction that is being constructed: i.e. the Pred and CC parts are not written as normal operands to instructions.
I guess we'd have to compare a substantial sample of all the lines of code where this pattern can be applied to see if this similarity between the assembly syntax of the instruction and the BuildMI syntax actually holds; or if it's just hand-wavy wishful thinking on my part.

Trying to do that exercise on one of the lines changed in this patch:

  AddDefaultCC(
      AddDefaultPred(BuildMI(MBB, MBBI, dl, TII.get(ARM::MOVr), ARM::SP)
                         .addReg(FramePtr)));

corresponds, I think, with the following instruction:

  MOV SP, FramePtr.

There is a clearer mapping between the BuildMI statement and the corresponding assembly instruction, I think, than with the alternative syntaxes discussed earlier on this review.

TBH, if we'd decide to stick to using addDefaultPred/CC, these functions could use some doxygen documentation.


https://reviews.llvm.org/D27984