This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Ensure PseudoLA* can be hoisted
ClosedPublic

Authored by jrtc27 on Mar 14 2022, 5:41 PM.

Details

Summary

Since we mark the pseudos as mayLoad but do not provide any MMOs,
isSafeToMove conservatively returns false, stopping MachineLICM from
hoisting the instructions. PseudoLA_TLS_GD does not actually expand to a
load, so stop marking that as mayLoad to allow it to be hoisted, and for
the others make sure to add MMOs during lowering to indicate they're GOT
loads and thus can be freely moved.

Fixes https://github.com/llvm/llvm-project/issues/54372

Diff Detail

Event Timeline

jrtc27 created this revision.Mar 14 2022, 5:41 PM
jrtc27 requested review of this revision.Mar 14 2022, 5:41 PM
craig.topper added inline comments.Mar 14 2022, 10:14 PM
llvm/test/CodeGen/RISCV/machinelicm-got.ll
8 ↗(On Diff #415286)

Is this TODO stale?

jrtc27 added inline comments.Mar 14 2022, 10:32 PM
llvm/test/CodeGen/RISCV/machinelicm-got.ll
8 ↗(On Diff #415286)

Oops, retroactively added it to the queued-up precommit (not committed yet, pending any review comments here of the tests themselves) but forgot to remove it here

MaskRay accepted this revision.Mar 14 2022, 11:49 PM
MaskRay added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
3745

Is it possible to construct a test that will compile to inferior assembly if MOInvariant is unset?

This revision is now accepted and ready to land.Mar 14 2022, 11:49 PM
MaskRay added inline comments.Mar 14 2022, 11:58 PM
llvm/test/CodeGen/RISCV/machinelicm-got.ll
1 ↗(On Diff #415286)

machinelicm-got.ll may not be the best name. I think it makes sense to place non-GOT tests in this file. For example test_lla doesn't use got.

arichardson accepted this revision.Mar 15 2022, 7:37 AM

LGTM

llvm/lib/Target/RISCV/RISCVInstrInfo.td
1340

This could be split into a separate commit but committing it at the same time also seems fine to me.

jrtc27 updated this revision to Diff 415687.Mar 15 2022, 8:44 PM
  • Renamed test case
  • Delete TODO as fixed
jrtc27 added inline comments.Mar 15 2022, 8:47 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
3745

I just copied what AMDGPU did. Other backends seem to bother less with all the attributes, I suspect they don't really matter even though technically correct.

llvm/test/CodeGen/RISCV/machinelicm-got.ll
1 ↗(On Diff #415286)

Yeah, originally it was just a PseudoLA test but then I later added PseudoLA_TLS_* and figured I should add PseudoLLA for completeness to show that one was already fine. Now renamed.

jrtc27 marked 2 inline comments as done.Mar 15 2022, 8:47 PM
MaskRay accepted this revision.Mar 16 2022, 11:23 AM
This revision was landed with ongoing or failed builds.Mar 16 2022, 11:46 AM
This revision was automatically updated to reflect the committed changes.
kamaub added a subscriber: kamaub.Mar 17 2022, 3:41 PM

I have confirmed that this patch cause a miscompile on our sanitizer-ppc64be-linux #4255 bot, please investigate and fix this as soon as possible, thank you.

I have confirmed that this patch cause a miscompile on our sanitizer-ppc64be-linux #4255 bot, please investigate and fix this as soon as possible, thank you.

The log says:

../../../../lib/libLLVMRISCVCodeGen.a(RISCVISelDAGToDAG.cpp.o): In function `selectImm(llvm::SelectionDAG*, llvm::SDLoc const&, llvm::MVT, long, llvm::RISCVSubtarget const&)':
RISCVISelDAGToDAG.cpp:(.text._ZL9selectImmPN4llvm12SelectionDAGERKNS_5SDLocENS_3MVTElRKNS_14RISCVSubtargetE+0x3d8): undefined reference to `llvm::SDValue llvm::RISCVTargetLowering::getAddr<llvm::ConstantPoolSDNode>(llvm::ConstantPoolSDNode*, llvm::SelectionDAG&, bool) const'
collect2: error: ld returned 1 exit status

Other buildbots are totally fine, as are my local builds. I really do not see how on earth _this_ change can cause the function I'm editing to no longer exist, unless the host toolchain is just broken.

My guess as to what the problem is is that this has perturbed inlining heuristics such that the ConstantPoolSDNode template instantiation no longer exists in the output object file, as we don't explicitly instantiate any of them, only implicitly via their uses?

Does:

diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index f52965dab759..299c68da7c2f 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -3769,6 +3769,15 @@ SDValue RISCVTargetLowering::getAddr(NodeTy *N, SelectionDAG &DAG,
   }
 }
 
+template SDValue RISCVTargetLowering::getAddr<GlobalAddressSDNode>(
+    GlobalAddressSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<BlockAddressSDNode>(
+    BlockAddressSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<ConstantPoolSDNode>(
+    ConstantPoolSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<JumpTableSDNode>(
+    JumpTableSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+
 SDValue RISCVTargetLowering::lowerGlobalAddress(SDValue Op,
                                                 SelectionDAG &DAG) const {
   SDLoc DL(Op);

fix the error for you?

kamaub added a comment.EditedMar 17 2022, 5:51 PM

Does:

diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index f52965dab759..299c68da7c2f 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -3769,6 +3769,15 @@ SDValue RISCVTargetLowering::getAddr(NodeTy *N, SelectionDAG &DAG,
   }
 }
 
+template SDValue RISCVTargetLowering::getAddr<GlobalAddressSDNode>(
+    GlobalAddressSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<BlockAddressSDNode>(
+    BlockAddressSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<ConstantPoolSDNode>(
+    ConstantPoolSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<JumpTableSDNode>(
+    JumpTableSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+
 SDValue RISCVTargetLowering::lowerGlobalAddress(SDValue Op,
                                                 SelectionDAG &DAG) const {
   SDLoc DL(Op);

fix the error for you?

I'm happy to report that this patch fixes the error:
I built f5ddcf25d67f3dbdbb486fbcf48c5247d4f59d6a without this change and I saw the failure then I built with this change and it completed without error.

I have confirmed that this patch cause a miscompile on our sanitizer-ppc64be-linux #4255 bot, please investigate and fix this as soon as possible, thank you.

The log says:

../../../../lib/libLLVMRISCVCodeGen.a(RISCVISelDAGToDAG.cpp.o): In function `selectImm(llvm::SelectionDAG*, llvm::SDLoc const&, llvm::MVT, long, llvm::RISCVSubtarget const&)':
RISCVISelDAGToDAG.cpp:(.text._ZL9selectImmPN4llvm12SelectionDAGERKNS_5SDLocENS_3MVTElRKNS_14RISCVSubtargetE+0x3d8): undefined reference to `llvm::SDValue llvm::RISCVTargetLowering::getAddr<llvm::ConstantPoolSDNode>(llvm::ConstantPoolSDNode*, llvm::SelectionDAG&, bool) const'
collect2: error: ld returned 1 exit status

Other buildbots are totally fine, as are my local builds. I really do not see how on earth _this_ change can cause the function I'm editing to no longer exist, unless the host toolchain is just broken.

I thought it was very strange as well, which is why I took so long to report it but the only thing that fixes the error is to revert the changeset.

Does:

diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index f52965dab759..299c68da7c2f 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -3769,6 +3769,15 @@ SDValue RISCVTargetLowering::getAddr(NodeTy *N, SelectionDAG &DAG,
   }
 }
 
+template SDValue RISCVTargetLowering::getAddr<GlobalAddressSDNode>(
+    GlobalAddressSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<BlockAddressSDNode>(
+    BlockAddressSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<ConstantPoolSDNode>(
+    ConstantPoolSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<JumpTableSDNode>(
+    JumpTableSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+
 SDValue RISCVTargetLowering::lowerGlobalAddress(SDValue Op,
                                                 SelectionDAG &DAG) const {
   SDLoc DL(Op);

fix the error for you?

I'd be happy to review and approve a patch with this change but I think the fastest resolution to getting the bot back to green would be to revert and then recommit with this change added. The bot has been broken for over 24 hours so we would like to get it back online as soon as possible, thank you for your quick response. :)

Does:

diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index f52965dab759..299c68da7c2f 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -3769,6 +3769,15 @@ SDValue RISCVTargetLowering::getAddr(NodeTy *N, SelectionDAG &DAG,
   }
 }
 
+template SDValue RISCVTargetLowering::getAddr<GlobalAddressSDNode>(
+    GlobalAddressSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<BlockAddressSDNode>(
+    BlockAddressSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<ConstantPoolSDNode>(
+    ConstantPoolSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<JumpTableSDNode>(
+    JumpTableSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+
 SDValue RISCVTargetLowering::lowerGlobalAddress(SDValue Op,
                                                 SelectionDAG &DAG) const {
   SDLoc DL(Op);

fix the error for you?

I'd be happy to review and approve a patch with this change but I think the fastest resolution to getting the bot back to green would be to revert and then recommit with this change added. The bot has been broken for over 24 hours so we would like to get it back online as soon as possible, thank you for your quick response. :)

I've just committed the fix as 63ea7797dd5bb77cc7b2904e20a5b779d30d4f2d; strange how this exposes it but it's been lurking ready to break since 41454ab25645 / D114950.

Does:

diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index f52965dab759..299c68da7c2f 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -3769,6 +3769,15 @@ SDValue RISCVTargetLowering::getAddr(NodeTy *N, SelectionDAG &DAG,
   }
 }
 
+template SDValue RISCVTargetLowering::getAddr<GlobalAddressSDNode>(
+    GlobalAddressSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<BlockAddressSDNode>(
+    BlockAddressSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<ConstantPoolSDNode>(
+    ConstantPoolSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+template SDValue RISCVTargetLowering::getAddr<JumpTableSDNode>(
+    JumpTableSDNode *N, SelectionDAG &DAG, bool IsLocal) const;
+
 SDValue RISCVTargetLowering::lowerGlobalAddress(SDValue Op,
                                                 SelectionDAG &DAG) const {
   SDLoc DL(Op);

fix the error for you?

I'd be happy to review and approve a patch with this change but I think the fastest resolution to getting the bot back to green would be to revert and then recommit with this change added. The bot has been broken for over 24 hours so we would like to get it back online as soon as possible, thank you for your quick response. :)

I've just committed the fix as 63ea7797dd5bb77cc7b2904e20a5b779d30d4f2d; strange how this exposes it but it's been lurking ready to break since 41454ab25645 / D114950.

Thank you very much, the bots is green now :)