[PATCH] D92716: [VE] Correct LVLGen (LVL instruction insert pass)

[Kazushi Marukawa via Phabricator via llvm-commits]

kaz7 planned changes to this revision.
kaz7 added a comment.

Updates.



================
Comment at: llvm/lib/Target/VE/LVLGen.cpp:55
+  if (Index >= 0) {
+    assert(MI.getOperand(Index).isReg() && "VL contains not a register!");
     return MI.getOperand(Index).getReg();
----------------
craig.topper wrote:
> getReg will assert this internally. Is this just to get a better error message?
This is not a really better error message.  I'll remove this assertion.  Thanks.


================
Comment at: llvm/lib/Target/VE/LVLGen.cpp:77
+    // possbile.  Therefore, we use a regular scalar register to hold immediate
+    // values to load VL register.  And try to reuse identical scalar regsiters
+    // and avoid new LVLr instructions as much as possible.
----------------
craig.topper wrote:
> regsiters->registers
Will do.  Thanks.


================
Comment at: llvm/lib/Target/VE/LVLGen.cpp:98
       } else {
+        // Reuse previous VL register.
         LLVM_DEBUG(dbgs() << "Reuse current VL.\n");
----------------
craig.topper wrote:
> This almost repeats the debug message but says "previous" instead of "current".
That's truth.  I'll remove comment.


================
Comment at: llvm/lib/Target/VE/LVLGen.cpp:108
+        // The latest VL is killed, so disable HasRegForVL.
         LLVM_DEBUG(dbgs() << RegName(RegForVL) << " is killed: ");
         LLVM_DEBUG(MI->dump());
----------------
craig.topper wrote:
> Is "killed" the right word to use here now?
Probablly, `need to be updated` is much better I think.

I was checking other implementations, and I noticed that I needed to check not only `definesRegister` but also `modifiesRegister`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92716