[llvm] [SPIRV] Audit `select` Result in SPIRVInstructionSelector (PR #115193)

Finn Plummer via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 12 10:28:14 PST 2024


================
@@ -1229,17 +1244,26 @@ bool SPIRVInstructionSelector::selectAtomicCmpXchg(Register ResVReg,
     const MachineMemOperand *MemOp = *I.memoperands_begin();
     unsigned Scope = static_cast<uint32_t>(getMemScope(
         GR.CurMF->getFunction().getContext(), MemOp->getSyncScopeID()));
-    ScopeReg = buildI32Constant(Scope, I);
+    auto ScopeConstant = buildI32Constant(Scope, I);
+    ScopeReg = ScopeConstant.first;
+    Result &= ScopeConstant.second;
 
     unsigned ScSem = static_cast<uint32_t>(
         getMemSemanticsForStorageClass(GR.getPointerStorageClass(Ptr)));
     AtomicOrdering AO = MemOp->getSuccessOrdering();
     unsigned MemSemEq = static_cast<uint32_t>(getMemSemantics(AO)) | ScSem;
-    MemSemEqReg = buildI32Constant(MemSemEq, I);
+    auto MemSemEqConstant = buildI32Constant(MemSemEq, I);
+    MemSemEqReg = MemSemEqConstant.first;
+    Result &= MemSemEqConstant.second;
     AtomicOrdering FO = MemOp->getFailureOrdering();
     unsigned MemSemNeq = static_cast<uint32_t>(getMemSemantics(FO)) | ScSem;
-    MemSemNeqReg =
-        MemSemEq == MemSemNeq ? MemSemEqReg : buildI32Constant(MemSemNeq, I);
+    if (MemSemEq == MemSemNeq)
+      MemSemNeqReg = MemSemEqReg;
+    else {
+      auto MemSemNeqConstant = buildI32Constant(MemSemEq, I);
+      MemSemNeqReg = MemSemNeqConstant.first;
+      Result &= MemSemNeqConstant.second;
----------------
inbelic wrote:

I agree it would be more elegant.

But from scanning the file, there doesn't seem to be a consistent precedent to checking a register's validity before use. In particular the returns of similar functions `buildZerosVal` and `BuildOnesVal`. I think to differentiate from those functions, and to ensure the result is checked in future calls we should explicitly return the type.

This might be a good reason to also audit the use of returned Registers, which I think is out of scope for this pr. Then we could switch to returning an invalid Register.

So I will merge as-is.

https://github.com/llvm/llvm-project/pull/115193


More information about the llvm-commits mailing list