[PATCH] D140952: Teach the AArch64 backend to materialize immediates using a pair of ORR-immediateinstructions.

[Dave Green via Phabricator via llvm-commits]

dmgreen accepted this revision.
dmgreen added a comment.
This revision is now accepted and ready to land.

Thanks. There are a couple of suggestions below, otherwise LGTM.



================
Comment at: llvm/lib/Target/AArch64/AArch64ExpandImm.cpp:345
+
+// Attempt to expand an immediate as a pair of ANDs of logical immediates.
+static bool tryAndOfLogicalImmediates(uint64_t UImm,
----------------
resistor wrote:
> dmgreen wrote:
> > "as an AND of an OR"? or a maybe explain the inverse of the immediate thing. I can see why this would work, but find it difficult to follow - it would be good to add a longer comment explaining how it works.
> The `ORR` is a red herring. The initial `ORR` is just a move-immediate, which the following `AND` then uses as its LHS.
Yeah. Its not really "as a pair of ANDs of logical immediates". It might be better to say " as an AND of a pair of logical immediates"


================
Comment at: llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:169
+    case AArch64::ANDXri:
+      if (I->Op1 == 0) {
+        MIBS.push_back(BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(I->Opcode))
----------------
resistor wrote:
> dmgreen wrote:
> > Can this happen?
> I'm not aware that anything creates it today, but I thought it was better to have the API of be orthogonal, so as to minimize surprise for any future folks touching this area.
An AND of 0 would just be a way of creating 0, and an AND would be a strange way of creating it. The Or with a WZR is the canonical form of a mov, but the And sounds like it would make less sense and might be a bug if it did come up. Perhaps change it to an assert that `I->Op1 != 0` instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140952