[PATCH] D45994: AMDGPU/GlobalISel: Enable TableGen'd instruction selector

[Tom Stellard via Phabricator via llvm-commits]

tstellar added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:478
+AMDGPUInstructionSelector::selectVSRC0(MachineOperand &Root) const {
+  auto Res = selectImm(Root);
+  if (Res != None)
----------------
tstellar wrote:
> nhaehnle wrote:
> > arsenm wrote:
> > > We don't fold immediates in the DAG selector now, so why do this here?
> > In fact, I remember there are some edge cases where folding immediates directly doesn't work due to moveToVALU. Basically, if you initially generate an instruction like S_ADD with an immediate and that instruction then gets moved to VALU it becomes a VOP3-encoded V_ADD which cannot have an immediate (because of the restriction that instructions can be at most 64 bits long), leading to machine instruction verifier errors.
> > 
> > The question is whether the same issue may apply here when going through GlobalISel in corner cases, even if we try to be better at selection the correct ALU directly. I think it makes sense to be a bit conservative initially.
> > We don't fold immediates in the DAG selector now, so why do this here?
> 
> Because we can?  Ideally doing folding in during ISel would allow us to eliminate or greatly simplify the SIFoldOperands pass, but admittedly, global-isel is so new I don't know if that would be possible.   I can go either way on folding during ISel, but it would at least be nice to have a target feature to enable this.
> 
> How do you feel about selecting directly to _e32 variants when we can instead of _e64, which also something that global-isel is doing?
> 
> 
> 
> 
> 
> 
>Because we can? Ideally doing folding in during ISel would allow us to eliminate or greatly simplify the SIFoldOperands pass, but admittedly, global-isel is so new I don't know if that would be possible. I can go either way on folding during ISel, but it would at least be nice to have a target feature to enable this.

Actually, thinking about this more I would prefer to have global-isel do immediate folding by default with a target option to disable it.  global-isel is still very experimental for AMDGPU, and I would like to take advantage of as many new features as possible while it's still in the experimental state.  This will also help us learn what's possible and what isn't.


================
Comment at: lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:478
+AMDGPUInstructionSelector::selectVSRC0(MachineOperand &Root) const {
+  auto Res = selectImm(Root);
+  if (Res != None)
----------------
tstellar wrote:
> tstellar wrote:
> > nhaehnle wrote:
> > > arsenm wrote:
> > > > We don't fold immediates in the DAG selector now, so why do this here?
> > > In fact, I remember there are some edge cases where folding immediates directly doesn't work due to moveToVALU. Basically, if you initially generate an instruction like S_ADD with an immediate and that instruction then gets moved to VALU it becomes a VOP3-encoded V_ADD which cannot have an immediate (because of the restriction that instructions can be at most 64 bits long), leading to machine instruction verifier errors.
> > > 
> > > The question is whether the same issue may apply here when going through GlobalISel in corner cases, even if we try to be better at selection the correct ALU directly. I think it makes sense to be a bit conservative initially.
> > > We don't fold immediates in the DAG selector now, so why do this here?
> > 
> > Because we can?  Ideally doing folding in during ISel would allow us to eliminate or greatly simplify the SIFoldOperands pass, but admittedly, global-isel is so new I don't know if that would be possible.   I can go either way on folding during ISel, but it would at least be nice to have a target feature to enable this.
> > 
> > How do you feel about selecting directly to _e32 variants when we can instead of _e64, which also something that global-isel is doing?
> > 
> > 
> > 
> > 
> > 
> > 
> >Because we can? Ideally doing folding in during ISel would allow us to eliminate or greatly simplify the SIFoldOperands pass, but admittedly, global-isel is so new I don't know if that would be possible. I can go either way on folding during ISel, but it would at least be nice to have a target feature to enable this.
> 
> Actually, thinking about this more I would prefer to have global-isel do immediate folding by default with a target option to disable it.  global-isel is still very experimental for AMDGPU, and I would like to take advantage of as many new features as possible while it's still in the experimental state.  This will also help us learn what's possible and what isn't.
In theory global-isel won't just be better at selecting the correct ALU, it will *always* select the correct ALU, so we shouldn't have to worry about corner cases like this,  but this is all in theory until we actually have a more complete global-isel implementation.


Repository:
  rL LLVM

https://reviews.llvm.org/D45994