[OpenACC] Reduction 'init' lowering for all-ones/least/largest #156535
Conversation
As a follow on to the last patches of this form, this patch does the init section for all of the reduction operators that weren't previously covered, which is '&' as all-ones, 'max' as 'least', and 'min' as 'largest'.
clang/lib/Sema/SemaOpenACC.cpp
Outdated
| case InitKind::AllOnes: | ||
| return llvm::APFloat::getAllOnesValue(Context.getFloatTypeSemantics(Ty)); | ||
| case InitKind::Least: | ||
| return llvm::APFloat::getSmallestNormalized( |
There was a problem hiding this comment.
Are you sure you don't want llvm::APFloat::getLargest(Context.getFloatTypeSemantics(Ty), /*Negative=*/true); here? I think getSmallestNormalized is going to return a value near zero.
There was a problem hiding this comment.
Ah! Good to know, thank you!
| // CHECK-NEXT: cir.store{{.*}} %[[LARGEST]], %[[DECAY]] : !cir.float, !cir.ptr<!cir.float> | ||
| // CHECK-NEXT: %[[ONE_IDX:.*]] = cir.const #cir.int<1> : !s64i | ||
| // CHECK-NEXT: %[[NEXT_ELT:.*]] = cir.ptr_stride(%[[DECAY]] : !cir.ptr<!cir.float>, %[[ONE_IDX]] : !s64i), !cir.ptr<!cir.float> | ||
| // CHECK-NEXT: %[[LARGEST:.*]] = cir.const #cir.fp<3.4{{.*}}> : !cir.float |
There was a problem hiding this comment.
Maybe also check for the E+38 here?
There was a problem hiding this comment.
Thats a good idea, I'm going to try to be more specific on all the FP checks other than 0, 1, all-ones.
| // TODO OpenACC: Expecting an initialization to... SOME value here. | ||
| // CHECK-NEXT: %[[ALLOCA:.*]] = cir.alloca !cir.array<!cir.float x 5>, !cir.ptr<!cir.array<!cir.float x 5>>, ["openacc.reduction.init", init] | ||
| // CHECK-NEXT: %[[DECAY:.*]] = cir.cast(array_to_ptrdecay, %[[ALLOCA]] : !cir.ptr<!cir.array<!cir.float x 5>>), !cir.ptr<!cir.float> | ||
| // CHECK-NEXT: %[[LEAST:.*]] = cir.const #cir.fp<-{{.*}}> : !cir.float |
There was a problem hiding this comment.
| // CHECK-NEXT: %[[LEAST:.*]] = cir.const #cir.fp<-{{.*}}> : !cir.float | |
| // CHECK-NEXT: %[[LEAST:.*]] = cir.const #cir.fp<-3.4{{.*}}E+38> : !cir.float |
| Exprs.push_back(FloatingLiteral::Create( | ||
| Context, getInitFloatValue(Context, IK, EltTy), | ||
| /*isExact=*/true, EltTy, ExprRange.getBegin())); | ||
| Exprs.push_back(FloatingLiteral::Create( |
There was a problem hiding this comment.
This probably isn't what you want. For example, for InitKind::One this will generate 1+1i but the identity value for complex multiplication is 1 + 0i. Maybe best to leave it as an error-producing case.
There was a problem hiding this comment.
Could you guide me as to what the right answer is for them? I could definitely split these up into separate functions, but I'd love to know what the '0', '1', All-Ones, Least and Largest should be here?
I think I'd one day soon want to enable complex here.
There was a problem hiding this comment.
I'm having a hard time picturing the use case for all-ones with floating point, but I suppose literally all ones for both components makes as much sense as anything. You could define starting points for minimum and maximum, but in general complex numbers can't be ordered in any meaningful sense. There is no < operator for complex values, for instance. When I googled this, I saw a few suggestions that you could compare the magnitude of the values, for which, largest or least for both component values would work. So I guess, InitKind::One is the only one for which what you did here wouldn't work. Trying to answer this question, on the other hand, clarified for me why the OpenACC standard wouldn't generally want to accept complex values.
There was a problem hiding this comment.
Ah, hmm.. ok then, I think leaving complex as ill-formed is the best idea.
I DID just realize my test before was wrong, and I HAVEN'T been disallowing complex, so I'm going to go through and do that as a RAC patch.
There was a problem hiding this comment.
Oops. Magnitude is an absolute value, so I guess you'd need {0, 0} for the LEAST case. Anyway, it's probably just a bad idea to allow it at all.
There was a problem hiding this comment.
I could probably make complex legal for some operators (+/*?), but it doesn't seem worth the effort until someone asks for it. In the meantime, I've made sure it is ill-formed always, here: 7a0dfb1
-Fix 'largest negative' for floats. -Make complex just emit 'zero', we are going to make it ill-formed, but it should produce a valid initializer at least -update all tests to properly check exponent for floats too.
As a follow on to the last patches of this form, this patch does the init section for all of the reduction operators that weren't previously covered, which is '&' as all-ones, 'max' as 'least', and 'min' as 'largest'.