[PATCH] Commoning of target specific load/store intrinsics

[Sanjin Sijaric]

Hi Pete and Hal,

Thank you for looking at this.  I attached a new patch to address your feedback.

Thanks,
Sanjin

-----Original Message-----
From: Pete Cooper [mailto:peter_cooper at apple.com] 
Sent: Saturday, January 17, 2015 1:25 PM
To: Hal Finkel
Cc: llvm-commits at cs.uiuc.edu; Sanjin Sijaric
Subject: Re: Commoning of target specific load/store intrinsics



Sent from my iPhone

> On Jan 17, 2015, at 7:02 AM, Hal Finkel <hfinkel at anl.gov> wrote:
> 
> ----- Original Message -----
>> From: "Pete Cooper" <peter_cooper at apple.com>
>> To: "Sanjin Sijaric" <ssijaric at codeaurora.org>
>> Cc: llvm-commits at cs.uiuc.edu, "Hal Finkel" <hfinkel at anl.gov>
>> Sent: Friday, January 16, 2015 3:50:46 PM
>> Subject: Re: Commoning of target specific load/store intrinsics
>> 
>> Hi Sanjin
>> 
>> 
>> I’m not sure if there’s a better way to handle this, but i have some 
>> general comments on the style in the patch.
>> 
>> 
>> + int MatchingId; // Same Id is set for corresponding load/store
>> intrinsics.
>> 
>> 
>> This should be an enum instead of an integer. Same goes for all other 
>> definitions and uses of this variable.
> 
> I don't see how this would work. The idea seems to be that the targets sets its own target-specific quantities here for which the actual definitions are visible only to the backend. So, in the backend implementation an enum would likely be used, but here it will need to be an int.
Ah yeah. My bad. Makes sense.

Thanks
Pete
> 
> -Hal
> 
>> 
>> 
>> 
>> + /// \returns An instruction defining the result of a memory
>> intrinsic. New
>> + /// instructions may be created to extract the result from an
>> intrinsic memory
>> + /// operation.
>> + virtual Value* getOrCreateResultFromMemIntrinsic(IntrinsicInst
>> *Inst,
>> 
>> 
>> You should mention that null is also a valid return value, if it was 
>> unable to get a valid intrinsic.
>> 
>> 
>> +bool AArch64TTI::getTgtMemIntrinsic
>> + (IntrinsicInst* Inst, MemIntrinsicInfo& Info) const {
>> 
>> 
>> …
>> 
>> 
>> +Value*
>> +TargetTransformInfo::getOrCreateResultFromMemIntrinsic(IntrinsicInst
>> *Inst,
>> + Type *ExpectedType)
>> + const {
>> 
>> 
>> Please run clang-format over the patch to see if there’s a better way 
>> to format long lines like these.
>> 
>> 
>> 
>> +Value* AArch64TTI::getOrCreateResultFromMemIntrinsic(IntrinsicInst*
>> Inst,
>> + Type* ExpectedType) const {
>> + Value* Res = nullptr;
>> + switch (Inst->getIntrinsicID()) {
>> + default:
>> + break;
>> ...
>> + return Res;
>> 
>> 
>> I would just return nullptr from the default case here. In fact i’d 
>> not have ‘Res’ at all in this method and just return from where you 
>> need to.
>> 
>> 
>> 
>> + case Intrinsic::aarch64_neon_st4:
>> + // Create a struct type
>> + StructType *ST = dyn_cast<StructType>(ExpectedType);
>> 
>> 
>> You should have { } over the body of this case as you’ve defined a 
>> variable here which is going to potentially break if more cases add 
>> their own variables.
>> 
>> 
>> 
>> + // Wrapper class to handle memory instructions, including loads,
>> stores and
>> + // intrinsic loads and stores defined by the target.
>> + class ParseMemoryInst {
>> 
>> 
>> /// and \brief to have the style of your other code.
>> 
>> 
>> + if (TTI->getTgtMemIntrinsic(II, Info)) { if (Info.NumMemRefs == 1) 
>> + { Store = Info.WriteMem; Load = Info.ReadMem; MayReadFromMemory = 
>> + Info.ReadMem; MayWriteToMemory = Info.WriteMem; Vol = Info.Vol; Ptr 
>> + = Info.PtrVal; } }
>> 
>> 
>> You can reduce nesting with 'if (!TTI->getTgtMemIntrinsic(II, Info)) 
>> return'
>> 
>> 
>> 
>> + ParseMemoryInst MemInst(Inst, TTI);
>> // If this is a non-volatile load, process it.
>> - if (LoadInst *LI = dyn_cast<LoadInst>(Inst)) {
>> + if (MemInst.isLoad()) {
>> 
>> 
>> I think here you need to first check if MemInst was constructed 
>> before you can safely check isLoad. You may want to add an isValid 
>> method and change it to ‘ MemInst.isValid() && MemInst.isLoad()’.
>> Probably similarly for later uses. Alternatively you could override 
>> bool operator() instead of defining isValid().
>> 
>> 
>> 
>> - if (Inst->mayReadFromMemory())
>> + if (MemInst.mayReadFromMemory())
>> 
>> 
>> This isn’t a valid change as the implementation of
>> Inst->mayReadFromMemory() handles things like Instruction::VAArg
>> which ParseMemoryInst doesn’t.

Thanks for catching this.  It's fixed in the updated patch.

>> 
>> 
>> Thanks,
>> Pete
>> 
>> 
>> 
>> On Jan 16, 2015, at 12:28 PM, Sanjin Sijaric < 
>> ssijaric at codeaurora.org > wrote:
>> 
>> 
>> 
>> Hi,
>> 
>> We have a need to common target specific load/store intrinsics (in 
>> our case, AArch64 intrinsics). This patch handles commoning of a few 
>> of these intrinsics in Early CSE. We modified the Early CSE a bit to 
>> get the necessary information from the target. Is there a more 
>> appropriate place to handle this other than Early CSE, or a better 
>> way to handle it in Early CSE?
>> 
>> Thanks,
>> Sanjin <CSEPATCH> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
-------------- next part --------------
A non-text attachment was scrubbed...
Name: CSEPATCH
Type: application/octet-stream
Size: 27732 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150119/a2b71aca/attachment.obj>