[PATCH] D63247: [ARM] MVE VPT Block Pass

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham added inline comments.


================
Comment at: llvm/lib/Target/ARM/Thumb2ITBlockPass.cpp:369
+
+      const unsigned Mask = 8U;
+      MIBuilder.addImm(Mask);
----------------
samparker wrote:
> SjoerdMeijer wrote:
> > samparker wrote:
> > > What's 8? Can we use an enum from somewhere?
> > That is the Mask Value (0b1000) for the VPST instruction as defined in the ARM ARM, see section B5.6. Predication/conditional execution. I'm not sure there's an enum for this, but will have a look. I will at least rename and comment on the Mask value here.
> Ah yes, cheers. Probably worth creating an enum for the values in Table Rvwgt and have it live where ever ARMVCC is defined.
I don't know of an enum for it, but as of the recent D63219, there's a comment explaining the format in `ARMAsmParser::ParseInstruction`.


================
Comment at: llvm/lib/Target/ARM/Thumb2ITBlockPass.cpp:363
+
+    if (getVPTInstrPredicate(*MI, PredReg) != ARMVCC::None) {
+      MachineInstrBuilder MIBuilder =
----------------
I suggest adding an assertion checking that the predicate is not `ARMVCC::Else`.

(The idea of that enum is that `None`, `Then` and `Else` are for use when handling assembly language: they correspond to the three possible suffixes "", "t" and "e" on the mnemonic. So when instructions are read from assembly source or disassembled from object code, you expect to see a mixture whenever there's a long VPT block. But in code generation, I //hope// we'll never generate an `Else` as input to this pass – though once this pass becomes able to generate nontrivial blocks, there will surely have to be `Else`-tagged instructions in its //output//, so that the `clang -S` output has the right suffixes on the mnemonics.


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

https://reviews.llvm.org/D63247