[Clang][OpenMP] Avoid using `IgnoreImpCasts` if possible

[tianshilei1992]

Summary

This patch removes all IgnoreImpCasts in Sema, and only uses it if necessary. If the expression is not of the same type as the pointer value, a cast is inserted.

[tianshilei1992]

ping

[ABataev]

Can you add the test?

[tianshilei1992]

In D126602#3548997, @ABataev wrote:

Can you add the test?

I don't have a good test off the top of my head.

[tianshilei1992]

I will refine some logics to set X, E, etc. to avoid more IgnoreImpCasts. However, I don't know how to deal with type promotion in the condition statement. Here we consider the following case:

if (x == e) { x = d; }

x and d can be set easily w/o need of IgnoreImpCasts. When it comes to e, things become complicated. There are basically three cases:

1. x and e are not promoted. That is the case when x and e are of same type, and they are both at least int. In this case, we don't need IgnoreImpCasts here. If they are of the same with shorter representation, such as int16_t, they will all be promoted, which will be mentioned later.
2. x is not promoted, but e is promoted. That is the case when x has higher ranking than e. In this case, we should not use IgnoreImpCasts.
3. Both x and e are promoted. This case is very common, and it can be further divided into following cases:
   • x and e of the same type with lower ranking. For example, when x and e are both short, they will be promoted to int. In this case, we want IgnoreImpCasts.
   • x and e are of different types, but they can be implicitly converted. For example, x is int, while e is short. In this case, we don't want IgnoreImpCasts because e is implicitly promoted to int, which is exactly what we want.

I'm thinking we may want to avoid using IgnoreImpCasts and later create cast if necessary. However, that may cause some unnecessary casts, at least in LLVM IR, such as the following case:

%1 = load i8 *%e
%2 = cast to i32 %1
%3 = cast to i8 %2
cmpxchg i8 * %x, i8 %3

I'm not sure eventually %2 and %3 will be optimized out.

[tianshilei1992]

update

[ABataev]

LG

clang/lib/CodeGen/CGStmtOpenMP.cpp

case BO_GT:
     Op = OMPAtomicCompareOp::MAX;
     break;
   default:
     llvm_unreachable("unsupported atomic compare binary operator");
   }
 
   LValue XLVal = CGF.EmitLValue(X);
   Address XAddr = XLVal.getAddress(CGF);
-  llvm::Value *EVal = CGF.EmitScalarExpr(E);
-  llvm::Value *DVal = D ? CGF.EmitScalarExpr(D) : nullptr;
+
+  auto EmitRValueWithCastIfNeeded = [&CGF, Loc](const Expr *X, const Expr *E) {
+    if (X->getType() == E->getType())
+      return CGF.EmitScalarExpr(E);
+    const Expr *NewE = E->IgnoreImplicitAsWritten();
+    llvm::Value *V = CGF.EmitScalarExpr(NewE);
+    if (NewE->getType() == X->getType())
+      return V;
+    return CGF.EmitScalarConversion(V, NewE->getType(), X->getType(), Loc);
+  };
+
+  llvm::Value *EVal = EmitRValueWithCastIfNeeded(X, E);
+  llvm::Value *DVal = D ? EmitRValueWithCastIfNeeded(X, D) : nullptr;
 
   llvm::OpenMPIRBuilder::AtomicOpValue XOpVal{
       XAddr.getPointer(), XAddr.getElementType(),
       X->getType()->hasSignedIntegerRepresentation(),
       X->getType().isVolatileQualified()};
   llvm::OpenMPIRBuilder::AtomicOpValue VOpVal, ROpVal;
 
   CGF.Builder.restoreIP(OMPBuilder.createAtomicCompare(

clang/lib/Sema/SemaOpenMP.cpp

if (!Cond) {
     ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = S->getCond()->getExprLoc();
     ErrorInfo.ErrorRange = ErrorInfo.NoteRange = S->getCond()->getSourceRange();
     return false;
   }
 
   switch (Cond->getOpcode()) {
   case BO_EQ: {
     C = Cond;
-    D = BO->getRHS()->IgnoreImpCasts();
+    D = BO->getRHS();
     if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS())) {
-      E = Cond->getRHS()->IgnoreImpCasts();
+      E = Cond->getRHS();
     } else if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
-      E = Cond->getLHS()->IgnoreImpCasts();
+      E = Cond->getLHS();
     } else {
       ErrorInfo.Error = ErrorTy::InvalidComparison;
       ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
       ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
       return false;
     }
     break;
   }
   case BO_LT:
   case BO_GT: {
-    E = BO->getRHS()->IgnoreImpCasts();
+    E = BO->getRHS();
     if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS()) &&
         checkIfTwoExprsAreSame(ContextRef, E, Cond->getRHS())) {
       C = Cond;
     } else if (checkIfTwoExprsAreSame(ContextRef, E, Cond->getLHS()) &&
                checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
       C = Cond;
       IsXBinopExpr = false;
     } else {
if (!Cond) {
     ErrorInfo.ErrorRange = ErrorInfo.NoteRange =
         CO->getCond()->getSourceRange();
     return false;
   }
 
   switch (Cond->getOpcode()) {
   case BO_EQ: {
     C = Cond;
-    D = CO->getTrueExpr()->IgnoreImpCasts();
+    D = CO->getTrueExpr();
     if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS())) {
-      E = Cond->getRHS()->IgnoreImpCasts();
+      E = Cond->getRHS();
     } else if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
-      E = Cond->getLHS()->IgnoreImpCasts();
+      E = Cond->getLHS();
     } else {
       ErrorInfo.Error = ErrorTy::InvalidComparison;
       ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
       ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
       return false;
     }
     break;
   }
   case BO_LT:
   case BO_GT: {
-    E = CO->getTrueExpr()->IgnoreImpCasts();
+    E = CO->getTrueExpr();
     if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS()) &&
         checkIfTwoExprsAreSame(ContextRef, E, Cond->getRHS())) {
       C = Cond;
     } else if (checkIfTwoExprsAreSame(ContextRef, E, Cond->getLHS()) &&
                checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
       C = Cond;
       IsXBinopExpr = false;
     } else {